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 up/down keys to inc/dec val in editor spin slider #41855

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

Razoric480
Copy link
Contributor

@Razoric480 Razoric480 commented Sep 7, 2020

Adds the ability to increment and decrement an integer while focused in the editor spin slider's line edit, via a gui_input event. Also works with shift to increment and decrement by x10, alt for x0.1, and control/command for x100.

Bugsquad edit: This closes godotengine/godot-proposals#29.

@Razoric480 Razoric480 changed the title Add up/down keys to inc/dec val in spin slider Add up/down keys to inc/dec val in editor spin slider Sep 7, 2020
@Calinou Calinou added this to the 4.0 milestone Sep 7, 2020
@Calinou
Copy link
Member

Calinou commented Sep 7, 2020

For consistency with web browsers' DevTools, we could use the same modifiers to increment/decrement values:

  • If they press up or down, we want to add or subtract 1
  • If they hold shift and press up or down, we want to add or subtract 10
  • If they hold alt and press up or down, we want to add or subtract 0.1
  • If they hold ctrl and press up or down, we want to add or subtract 100. On Mac, we want to use the cmd key for consistency.

https://kilianvalkhof.com/2020/javascript/supercharging-input-type-number/

@Razoric480
Copy link
Contributor Author

Razoric480 commented Sep 7, 2020

I can add the extra modifiers, sure!

How does DevTools handle integer values and alt?

Looks like Ctrl/Cmd has priority, shift second, then alt, and alt is ignored on integers.

@Razoric480
Copy link
Contributor Author

Razoric480 commented Sep 7, 2020

An issue occurs with using devtools style is that in GDScript, you can export a variable with a given step. I.E.

@export_range(0, 100, 0.5)
var my_exported_range = 50.0

So the question occurs: if pressing up does 0.5, if the user holds down CTRL/CMD, SHIFT or ALT and presses up, how does the number react? 50.0, 5.0, 0.5, 0.05?

The default step for floating point numbers is 0.001, furthermore. Unless we force the values to 100, 10, 1, 0.1, that makes even ctrl/cmd have only a small effect.

EDIT: One suggestion is to make modifier-less up and down use step, and the rest go with their devtool style of 100, 10 and 0.1.

@Razoric480 Razoric480 force-pushed the key_up_down_spin_slider branch 2 times, most recently from 5079960 to 6d2e332 Compare September 7, 2020 21:14
@coppolaemilio
Copy link
Member

Any chance we can seem some action here again? I still press up and down when selecting items in the inspector and I always remember about this PR 🙈

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested on the latest master branch (after rebasing), it works as intended. Looks good from an UX and implementation perspective.

However, with floating-point properties, changing the value using Up/Down without holding any modifiers will change the value by the smallest allowed step (typically 0.001). We should change this so that the step is always 1 when not using a modifier, for consistency with integer properties.

@Razoric480
Copy link
Contributor Author

The issue with that is that it steps on the toes of the export hint. I.E.:

export(float, 100, 1000, 20) var value: float

will make up and down go by steps of 20 because the inspector has it as its value. If someone sets theirs to 0.25, but we force it to 1, that creates a dissonance.

@Razoric480 Razoric480 closed this Jul 8, 2021
@Razoric480 Razoric480 deleted the key_up_down_spin_slider branch July 8, 2021 13:15
@Razoric480 Razoric480 restored the key_up_down_spin_slider branch July 8, 2021 13:15
@Razoric480 Razoric480 reopened this Jul 8, 2021
@Razoric480 Razoric480 force-pushed the key_up_down_spin_slider branch 2 times, most recently from a4dc8c0 to 35d39af Compare July 8, 2021 13:33
@Calinou
Copy link
Member

Calinou commented Jul 15, 2021

will make up and down go by steps of 20 because the inspector has it as its value. If someone sets theirs to 0.25, but we force it to 1, that creates a dissonance.

For the keyboard shortcuts, I was thinking we should use "our" float step of 1 whenever the property's float step is an integer divisor of 1 (1/2 = 0.5, 1/4 = 0.25, 1/8 = 0.125, …). The default float step (0.001) is 1/1000, so it works here 🙂

When a modifier key tries to set a value not allowed by the float step, it tries to round using trunc() + float_step when increasing the property, and trunc() when decreasing the proprerty. (trunc() rounds towards negative infinity, unlike floor() which rounds towards zero.)

If it's not a divisor, we should use the property's float step instead.

@Razoric480 Razoric480 force-pushed the key_up_down_spin_slider branch 2 times, most recently from cb222e8 to 5194de4 Compare August 23, 2021 19:04
@Razoric480 Razoric480 marked this pull request as draft August 23, 2021 21:16
@Razoric480 Razoric480 force-pushed the key_up_down_spin_slider branch 2 times, most recently from 47e0ab7 to e36fd69 Compare September 26, 2021 14:54
@Razoric480 Razoric480 marked this pull request as ready for review September 26, 2021 14:55
@Razoric480
Copy link
Contributor Author

Finally got back to this. As suggested by Calinou, it will treat <1 values that are integer divisors as 1, though will use the real step for >1 or when the step causes an invalid value (I.E. a step of 0.25 trying to increase by 0.1 would cause no increase otherwise.)

editor/editor_spin_slider.cpp Outdated Show resolved Hide resolved
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

LGTM, I did not get perfectly the logic for choosing hte right value, but the implementation looks good otherwise.

@akien-mga akien-mga merged commit 06b9ea1 into godotengine:master Sep 28, 2021
@akien-mga
Copy link
Member

Thanks!

@Razoric480 Razoric480 deleted the key_up_down_spin_slider branch September 28, 2021 16:21
@Razoric480 Razoric480 restored the key_up_down_spin_slider branch September 29, 2021 18:13
@Razoric480 Razoric480 deleted the key_up_down_spin_slider branch October 5, 2021 14:32
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.

Add "+/-" shortkeys to edit variables in inspector
6 participants