Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Controllable AutoShowMetal #1851

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

saurtron
Copy link
Collaborator

@saurtron saurtron commented Dec 24, 2024

Work done

  • Make guiHandler::autoShowMetal controllable through lua.

Related Issues

Remarks

See lua-callin-activecommand-set...rid-of-autoshowmetal for a direct comparison to parent (lua-callin-activecommand-set) branch.

  • Branch on top of Add ActiveCommandChanged callin. #1848, leaving this as DRAFT until that one is merged just to avoid this one being merged before, since it includes that one.
    • Had to base on that branch since it changes very similar lines.
  • Could be removed altogether but this makes it backwards compatible.

@saurtron saurtron marked this pull request as draft December 24, 2024 10:29
@sprunk
Copy link
Collaborator

sprunk commented Dec 26, 2024

Just get rid of it, the ticket contains a lua replacement. Ideally there would be a feature support tag and a bunch of forward-compatible PRs to games.

@saurtron
Copy link
Collaborator Author

saurtron commented Dec 26, 2024

Just get rid of it, the ticket contains a lua replacement. Ideally there would be a feature support tag and a bunch of forward-compatible PRs to games.

I think breaking compatibility as default policy, even if we PR to every game is not very good in general. We might not know all games (some could be private) and it puts an urgent burden on engine users (also on engine devs if we're expected to PR everyone).

Personally would rather make AutoShowMetal show a deprecated message so ppl have more time to adapt, and then in a future release (maybe even after next release), remove it.

@lhog
Copy link
Collaborator

lhog commented Jan 4, 2025

I don't think we need to rid of anything. Like who is going redo 100s of maps to include #1092 (comment) widget or who is going to create configs for the same 100s of maps per each game? This is just to replicate the previous behavior.

Seems like a bunch of work (that no one is going to do) for the very little benefit.

I'd keep LuaUnsyncedCtrl::SetAutoShowMetal() but remove all deprecation warnings and keep the default coming from the map, same as now.

@saurtron
Copy link
Collaborator Author

saurtron commented Jan 5, 2025

I don't think we need to rid of anything. Like who is going redo 100s of maps to include #1092 (comment) widget or who is going to create configs for the same 100s of maps per each game? This is just to replicate the previous behavior.

Seems like a bunch of work (that no one is going to do) for the very little benefit.

I'd keep LuaUnsyncedCtrl::SetAutoShowMetal() but remove all deprecation warnings and keep the default coming from the map, same as now.

Added an example widget showing usage: gui_autoshowmetal.lua

Using that gets rid of the deprecation message, due to the call to SetAutoShowMetal(false).

It is also backwards compatible with map declaration, starting there games can implement new policies and then get rid of autoshowmetal in mapinfo if they so desire.

In a future engine release version after SetAutoShowMetal has existed for at least one cycle, we can completely remove autoshowmetal logic from GuiHandler.cpp.

@lhog
Copy link
Collaborator

lhog commented Jan 5, 2025

Sounds reasonable.

@saurtron saurtron marked this pull request as ready for review January 8, 2025 23:39
rts/Lua/LuaConstEngine.cpp Outdated Show resolved Hide resolved
Comment on lines 21 to 22
function widget:Initialize()
if Spring.SetAutoShowMetal then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should include forward-compat as well. This is so that games can add the widget before migrating in a fire-and-forget way, and it won't be applying conflicting changes in parallel with the native engine autometal behaviour.

Suggested change
function widget:Initialize()
if Spring.SetAutoShowMetal then
function widget:Initialize()
if not Engine.FeatureSupport.noAutoShowMetal then
-- engine hasn't yet removed autometal and handles everything for us! arigato mr. Recoil 🙏
widgetHandler:RemoveWidget()
return
end
if Spring.SetAutoShowMetal then

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to check for nil since it's now a bool and want it to not trigger the deprecation warning once this is in, so SetAutoShowMetal needs to be called when it's defined in general.

Copy link
Collaborator Author

@saurtron saurtron Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...so added a variant of your suggestion.

rts/Lua/LuaUnsyncedCtrl.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of AutoShowMetal
3 participants