-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
[3.x] Make TextureButton
and Button
update on texture change
#81113
[3.x] Make TextureButton
and Button
update on texture change
#81113
Conversation
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.
Logic slightly changes, as in 3.x we didn't check if (destination == p_texture)
for TextureButton textures before. So with this change there won't be updates triggered if someone sets the exactly the same value to a member.
I think it should be fine though. And the code adaptation to 3.x LGTM.
ed1ffbd
to
867145f
Compare
Can remove the check for same value as well if wanted to keep parity with 3.x, though the changes to |
867145f
to
d0a98e1
Compare
@@ -343,6 +344,27 @@ void TextureButton::set_focused_texture(const Ref<Texture> &p_focused) { | |||
focused = p_focused; | |||
}; | |||
|
|||
void TextureButton::_set_texture(Ref<Texture> *p_destination, const Ref<Texture> &p_texture) { | |||
DEV_ASSERT(p_destination); |
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.
DEV_ASSERT
should only be used for bottleneck code. A regular check here won't matter.
But I think a better way is to pass by non-const reference, so we don't have to do any check.
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.
See: #77159 (comment)
Also DEV_ASSERT
is for crashing, not performance critical AFAIK
Keeping the code as proven in 4.x for now
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.
That won't work, assigning to the reference won't update the texture itself
The referenced Ref
object won't change. What will be updated is the content of that Ref
, which is the texture.
Also
DEV_ASSERT
is for crashing, not performance critical AFAIK
A note was added recently:
godot/core/error/error_macros.h
Lines 797 to 801 in 8449592
* DEV_ASSERT should generally only be used when both of the following conditions are met: | |
* 1) Bottleneck code where a check in release would be too expensive. | |
* 2) Situations where the check would fail obviously and straight away during the maintenance of the code | |
* (i.e. strict conditions that should be true no matter what) | |
* and that can't fail for other contributors once the code is finished and merged. |
But anyway, I think it's fine if the Godot 4 version uses this. I should have read the original PR, sorry :(
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.
No problem, all the changes except to fit it to 3.x code are from the 4.x one, now minus the is_valid
part
Checked in 4.x and passing by reference works there, but don't want any potential unseen side effects for a cheery pick, thank you for feedback
Thanks! |
Thank you! |
Cherry-picked for 3.5.3. |
TextureButton
andButton
update on texture change #77159Currently unable to verify functionality on 3.x builds, code should work but would appreciate someone verifying