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

Add second alternate speed and buttons for alt speed #11187

Merged
merged 11 commits into from
Jun 24, 2018

Conversation

unknownbrackets
Copy link
Collaborator

@unknownbrackets unknownbrackets commented Jun 17, 2018

This refactors a bunch of things along the way. The more user facing things are:

  1. There are now two alternate speed settings. Each can be disabled.
  2. Speed toggle cycles through alt speed settings (normal -> alt speed 1 -> alt speed 2 -> normal.)
  3. There are two new buttons for "alt speed 1" and "alt speed 2". These function like "unthrottle" in that the speed is applied while they are pressed.
  4. The "speed 1" and "speed 2" buttons are also available as touch buttons.

I think this should satisfy the needs in #7981, at least generally. Rather than change unthrottle (which does what it says and is simple), to me it makes more sense to make the alt speed option more versatile as the above.

There's also some more behind the scenes changes:

  1. Moved some config values into ConfigValues.h - a lot of places didn't use enums, or have hidden away enums, and I feel like this will encourage us to write cleaner code (since ConfigValues.h is included far less places.)

  2. All the touch control related settings are now grouped in structs, which makes the code simpler and repeat less. There's still parts that could be better, but at least it's much easier to add a new button now.

  3. Made the touch control visibility page use more consistent UI with touch control layout and fixed a leaked class per checkbox.

  4. Sorts the touch control visibility list as it was probably intended to be sorted (otherwise "Alt speed 1" would be first.)

  5. Removed old pre-0.9.8 or so touch control layout config migration code.

-[Unknown]

@unknownbrackets unknownbrackets added this to the v1.7.0 milestone Jun 17, 2018
@unknownbrackets
Copy link
Collaborator Author

Oh, the buttons are kinda uninspired right now, just 10 degree upward arrow and 10 degree downward arrow. I'm no artist. Showing a 1 and 2 in them sounded worse...

-[Unknown]

@unknownbrackets unknownbrackets force-pushed the alt-speed branch 4 times, most recently from df875b9 to 8b2e181 Compare June 17, 2018 08:30
@hrydgard
Copy link
Owner

Cool, will read the code soon. My only immediate objection is that buttons/controls that cycle through more than two settings often feel awkward and confusing to use, and if one of the speeds is super fast can easily skip forward much more than intended by accident trying to toggle back to normal speed for example.

@unknownbrackets
Copy link
Collaborator Author

unknownbrackets commented Jun 17, 2018

There's no cycle touch button - there's only two buttons for activating each speed.

You can however map a gamepad / keyboard button to toggle between the speeds (same toggle button as before.) If the second speed is disabled (as is true by default), it continues working as before: toggling on and off the first alternative speed.

That has never had a touch button, though.

So there are now 4 total buttons:

  • Unthrottle (while pressed) - touch or gamepad, as before
  • Alt speed toggle (toggles between speeds) - gamepad only, as before (it just now toggles between 3, instead of 2, settings)
  • Alt speed 1 (while pressed) - touch or gamepad, new
  • Alt speed 2 (while pressed) - touch or gamepad, new

-[Unknown]

@hrydgard
Copy link
Owner

Ah, I missed that disabling the speeds would remove them from the cycle. That should be alright then as it's easy to set it up with a single alternate speed too.

@unknownbrackets
Copy link
Collaborator Author

unknownbrackets commented Jun 17, 2018

Right - I almost created two toggles but it seemed hard to explain in the UI. I think the "while pressed only" behavior is probably what is most logical for the current touch interface and for e.g. trigger buttons.

There might be an argument to make any touch button a "toggle button" by making it lock when tapped and unlock when tapped again.

-[Unknown]

@unknownbrackets
Copy link
Collaborator Author

What do you think? I'd like to recommend that #11202 add an enum to ConfigValues for the magic numbers, but it doesn't exist yet.

-[Unknown]

Can be used for slow motion or fast motion (esp. if unthrottle is too
fast.)
These are a bit strewn about and there are constants that aren't
consistently used, which just adds confusion.
For alternate speed, we'll allow separate speeds to be "on" or "off".
In addition to virtual keys for each speed separately.
Much cleaner this way, less repetition.
This allows more flexibility if unthrottle is too fast or too uneven.
Now it looks more like other screens.
@hrydgard
Copy link
Owner

Yeah, sorry for the delay, I'm fine with this. Will just read through it again then merge.

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.

2 participants