-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Fix nine patch of circular TextureProgressBar #54345
Fix nine patch of circular TextureProgressBar #54345
Conversation
@@ -450,48 +450,43 @@ void TextureProgressBar::_notification(int p_what) { | |||
} | |||
|
|||
float val = get_as_ratio() * rad_max_degrees / 360; | |||
if (val == 1) { | |||
Rect2 region = Rect2(progress_offset, s); | |||
Rect2 source = Rect2(Point2(), s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just changing that line to:
Rect2 source = Rect2(Point2(), progress->get_size());
should fix the case when val == 1
which seemed to be the problem in here. And that case seems to be simpler/faster so I'd opt for keeping it in (instead of removing it like you did).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kleonc Yes, you're right. Better keep it this way.
@kleonc How about removing this block: if (uvs.find(uv) >= 0) {
continue;
} Due to float precision, find() can't work properly here. Might cause some future issue. |
This block prevents adding twice exactly the same godot/scene/gui/texture_progress_bar.cpp Line 489 in d046817
Due to float precision two similar uv values might possibly end up in the uvs but it should be fine as long as generated points form a valid polygon (so it could be rendered properly). And if generated polygon can currently indeed end up being invalid due to float precision then removing the block you've mentioned won't fix it anyway, it would only allow exactly the same uv /point pairs to occur.I'm not sure if two exactly the same uv can be produced though. It could possibly occur only in case when start /end is almost equal to one of the corners and unit_val_to_uv(start) /unit_val_to_uv(end) would produce the same uv as unit_val_to_uv(that_corner) . But again, not sure if this can happen. If it's impossible then the block you've mentioned could indeed be removed as that condition would never be met; otherwise it handles such situation.So I'd just leave it as is. It doesn't do any harm besides adding a small overhead ( uvs vector will have at most 7 elements).
Some notes after looking at the code (kinda out of the scope of this PR but I guess you could include them in here if you want):
Also
could be changed to:
|
5f37415
to
f9b1ce0
Compare
@kleonc Good idea. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pts
are still Array
but it's unrelated to the bugfix in here so it can be changed in another PR.
I've checked again math in the corner
calculations (which are also unrelated to the bugfix) and it looks correct.
Should be fine to merge.
Thanks! |
Cherry-picked for 3.5. |
@akien-mga Should this be cherry-picked to 3.4.3? |
Cherry-picked for 3.4.3. |
With nine patch enabled.
Before:
After: