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

Auto spec menu #9468

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

Auto spec menu #9468

wants to merge 3 commits into from

Conversation

TsFreddie
Copy link
Contributor

@TsFreddie TsFreddie commented Jan 3, 2025

Depends on #9466. Check commit for diff from that PR.

Full video demo: https://streamable.com/zva2i4

After some feedback on 18.9 RC, and given how confusing and polarizing the feature is, i think it is now pretty important to add a visible toggle and a permanent setting (cl_spec_auto_sync, please check the description on whether it is clear enough)

Adds a in-game menu button for toggling this feature (it toggles both ingame state and the config, only shown in spectator mode)
image

Along with these status tooltips:

  • Active
  • Inactive
  • Disabled
  • Enabled (when it is not possible to activate)
  • Unavailable for this player (when spectating older clients))

This is quite a few localization strings but I think it is worth it for clarity.

This button toggles between three or two states depends on the context. Either Active - Inactive - Disabled or just Enabled - Disabled.

When the feature is not disabled, assume player do not mind the feature and currently taking Skeith suggestion to automatically activate when player resets zoom, and deactivate after zoom+ or zoom-. I did not check whether current zoom is default because:

  1. it is hard to do due to floating point comparison
  2. it is harder to guess player's intention why they just zoomed out and zoomed in)

Also updated the indicator icon's color to match the button and make it less distracting. And it will completely hide if player disabled the feature.
image

I'd like this to be in 18.9, even though this is a pretty big change, but it came from discord feedback. Review carefully.

Demo menu is unchanged because I think it doesn't make sense to follow the config.

Checklist

  • Tested the change ingame
  • Provided screenshots if it is a visual change
  • Tested in combination with possibly related configuration options
  • Written a unit test (especially base/) or added coverage to integration test
  • Considered possible null pointers and out of bounds array indexing
  • Changed no physics that affect existing maps
  • Tested the change with ASan+UBSan or valgrind's memcheck (optional)

@Robyt3 Robyt3 added this to the DDNet 18.9 milestone Jan 3, 2025
Copy link
Member

@def- def- left a comment

Choose a reason for hiding this comment

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

We could also move the entire feature to 19.0 to give it more time to brew.

@TsFreddie
Copy link
Contributor Author

We could also move the entire feature to 19.0 to give it more time to brew.

like cherry-picking or a hard code bypass

@def-
Copy link
Member

def- commented Jan 3, 2025

Reverting it in 18.9, but keeping it in master

@TsFreddie
Copy link
Contributor Author

Reverting it in 18.9, but keeping it in master

sure, we can try that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants