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

Draw the filled part of the slider on float EditorSpinSliders #55519

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Dec 1, 2021

master version of #55521.

This makes it more obvious that the slider has a "filled" part on the left, which improves visibility especially in wider inspectors (such as the Project Settings and Editor Settings).

Preview

Before After
2021-12-01_19 03 21 image

This makes it more obvious that the slider has a "filled" part
on the left, which improves visibility especially in wider inspectors
(such as the Project Settings and Editor Settings).
@Zireael07
Copy link
Contributor

Whoa, I never realized the sliders have a filled part! My poor eyes say THANKS!!!

@akien-mga
Copy link
Member

akien-mga commented Dec 1, 2021

That looks pretty cool for ratios definitely. But what about arbitrary float sliders which do not range from 0 to 1? Is the "filled"/"progress" concept still relevant?

@akien-mga akien-mga requested a review from a team December 1, 2021 23:00
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Only reviewed the photos. The feature improves usability.

Edited:

May want to tune the scale of the contrast of the three portions for OLED screens and etc.

@YuriSizov
Copy link
Contributor

YuriSizov commented Dec 2, 2021

I have the same concern as Akien. It sure makes semantic sense in some situations, but not in others. Maybe it should be an option and we should only enable it for properties that have PROPERTY_HINT_RANGE with both ends?

Edit: Though, I guess the slider itself is already only visible in such cases. Probably good then.

@akien-mga
Copy link
Member

Maybe it should be an option and we should only enable it for properties that have PROPERTY_HINT_RANGE with both ends?

Edit: Though, I guess the slider itself is already only visible in such cases. Probably good then.

Well my point is that sometimes the range we specify is not the range of absolute values that something can take, but a generic "wide yet small enough" range for a slider to be a meaningful way to pick a value.

For example:

scene/resources/cylinder_shape_3d.cpp
100:    ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "radius", PROPERTY_HINT_RANGE, "0.001,4096,0.001"), "set_radius", "get_radius");
101:    ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "height", PROPERTY_HINT_RANGE, "0.001,4096,0.001"), "set_height", "get_height");

It doesn't make much sense to represent 0.001 radius as 0% filled and 4096 radius as 100% filled UX wise IMO.

That being said, I'm not sure if it's actually detrimental to UX as opposed to just being superfluous in this case.

@YuriSizov
Copy link
Contributor

IMO that's probably fine. or_greater and or_lesser spinners would be more problematic (the ones you refer to seem to have a fixed range). I'm not sure I'm a fan of the way we show the slider at all. But this improvement makes it ever so slightly easier to identify visually what the overall state is.

@akien-mga akien-mga merged commit b46ab89 into godotengine:master Dec 10, 2021
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the editor-spin-slider-float-draw-filled-slider branch December 10, 2021 18:34
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.

5 participants