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

Make ProgressBar reflect the real value in the percent label when "allow greater" is checked #83623

Merged

Conversation

rarysson
Copy link
Contributor

@rarysson rarysson commented Oct 19, 2023

Fix #63311.

I have just a question. When we change the min value or the max value, the current value is updated if it is above the max value or below the min value. Should I implement this behavior when the user toggles Allow Greater/Lesser? Right now if I toggle Allow Greater/Lesser it will keep the old value, as you can see in the video below.

progressbar.mp4

It seems because in the Range class it doesn't emit the change like the other properties (e.g.: page, max, min, etc.).

godot/scene/gui/range.cpp

Lines 360 to 362 in f8818f8

void Range::set_allow_greater(bool p_allow) {
shared->allow_greater = p_allow;
}

@rarysson rarysson requested a review from a team as a code owner October 19, 2023 15:40
@KoBeWi
Copy link
Member

KoBeWi commented Oct 19, 2023

You can't just divide by max, you need to take min into account also avoid dividing by 0.

@rarysson rarysson force-pushed the label-progressbar-allow-greater branch 2 times, most recently from f527c34 to 0a493e8 Compare October 19, 2023 16:57
@rarysson
Copy link
Contributor Author

It was too good to be right on the first try haha.

Now I just need to know if I need to fix the behavior that I mentioned in the video, or if it's expected.

@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 31, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Nov 7, 2023

It breaks exp_edit though:
image
I think instead of inventing custom ratio, get_as_ratio() should be modified to allow values outside the 0-1 range if allow_greater/allow_lesser are enabled. The value is explicitly clamped, so it should be easy change.

It seems because in the Range class it doesn't emit the change like the other properties

This is irrelevant, because changing these properties does not affect current value.

@rarysson
Copy link
Contributor Author

rarysson commented Nov 8, 2023

You're right, it would be much simpler, but I don't think we can do that safely, because some UI elements rely on the value from get_as_ratio(), and it will break if the value is outside of the 0-1 range, as you can see below:

image

I just have one question, if the exp_edit and allow_lesser are checked what is the expected behavior if the value is less than or equal to 0?

@rarysson rarysson force-pushed the label-progressbar-allow-greater branch from 0a493e8 to b4ccea8 Compare November 10, 2023 01:57
@KoBeWi
Copy link
Member

KoBeWi commented Nov 10, 2023

I just have one question, if the exp_edit and allow_lesser are checked what is the expected behavior if the value is less than or equal to 0?

exp_edit requires the min_value to be >= 0, so I think the exp ratio should be always 0 when below that.

btw "exp_edit requires the min_value to be >= 0," is not respected by your code. I think the implementation is ok, just make sure that the percentage always matches the filled bar, i.e. 50% should be when the bar is half filled etc. Right now it's still possible to desync it.

@rarysson rarysson force-pushed the label-progressbar-allow-greater branch from b4ccea8 to acec1f9 Compare November 10, 2023 18:01
@rarysson
Copy link
Contributor Author

[...] I think the implementation is ok, just make sure that the percentage always matches the filled bar, i.e. 50% should be when the bar is half filled etc. Right now it's still possible to desync it.

To be honest, I don't see how it could desync, because it's the same logic as the get_as_ratio(), do you have an example?

I tried to see if it behaved differently from the stable version, but I don't see the difference, as you can see below (I tried some different values too).

Godot 4.1

4 1

This PR

mine

@KoBeWi
Copy link
Member

KoBeWi commented Nov 10, 2023

Ok the only one I could find was with exp_edit (again xd) and negative min_value:

godot.windows.editor.dev.x86_64_wPldJzsHBo.mp4

Other than that the value displays correctly.

@rarysson rarysson force-pushed the label-progressbar-allow-greater branch 4 times, most recently from c424cb1 to 57a0653 Compare November 11, 2023 02:14
@rarysson
Copy link
Contributor Author

Now I think it works as expected.

@rarysson rarysson force-pushed the label-progressbar-allow-greater branch from 57a0653 to beeca2a Compare November 11, 2023 14:18
@YuriSizov YuriSizov merged commit 3a44484 into godotengine:master Dec 20, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks, and congrats on your first merged Godot contribution!

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.

ProgressBar should reflect "allow greater" values in the percent label
4 participants