Add check in AnimationNodeOneShot during fade-out to ensure signal fires#103078
Closed
BlackShift wants to merge 1 commit into
Closed
Add check in AnimationNodeOneShot during fade-out to ensure signal fires#103078BlackShift wants to merge 1 commit into
AnimationNodeOneShot during fade-out to ensure signal fires#103078BlackShift wants to merge 1 commit into
Conversation
BlackShift
changed the title
AnimationNodeOneShot during fade-out to ensure signal fires
AThousandShips
added
bug
topic:animation
cherrypick:4.3
TokageItLab
requested changes
May 18, 2025
|
|
||
| if (!p_seek) { | ||
| if (Animation::is_less_or_equal_approx(os_nti.get_remain(break_loop_at_end), 0) || (is_fading_out && Animation::is_less_or_equal_approx(cur_fade_out_remaining, 0))) { | ||
| if (Animation::is_less_or_equal_approx(os_nti.get_remain(break_loop_at_end), 0) || (is_fading_out && Animation::is_less_or_equal_approx(cur_fade_out_remaining, 0) && os_nti.length == os_nti.position)) { |
Member
There was a problem hiding this comment.
Suggested change
| if (Animation::is_less_or_equal_approx(os_nti.get_remain(break_loop_at_end), 0) || (is_fading_out && Animation::is_less_or_equal_approx(cur_fade_out_remaining, 0) && os_nti.length == os_nti.position)) { | |
| if (Animation::is_less_or_equal_approx(os_nti.get_remain(break_loop_at_end), 0)) { |
Note that os_nti.length == os_nti.position is (mostly) same with os_nti.get_remain(break_loop_at_end) == 0. This means that there is no need for a special check during fading.
I don't remember what this check is for, but considering the issue, this fix seems to make sense. I suspect there probably won't be any major side effects, so I think we should merge the changes as soon as they are made and see if any new issue arise.
Member
There was a problem hiding this comment.
Oh wait, there is a side effect that Shot's State remains when FadeOut is requested. So I feel that perhaps the timing should be shifted or something.
Member
There was a problem hiding this comment.
Suggested change
| if (Animation::is_less_or_equal_approx(os_nti.get_remain(break_loop_at_end), 0) || (is_fading_out && Animation::is_less_or_equal_approx(cur_fade_out_remaining, 0) && os_nti.length == os_nti.position)) { | |
| if (Animation::is_less_or_equal_approx(os_nti.get_remain(break_loop_at_end), 0) || (is_fading_out && cur_fade_out_remaining <= 0)) { |
So this improves accuracy, but it seems insufficient due to problems elsewhere. I will look into it when I have time.
Member
Contributor
Author
|
Closing in favor of #106567 |
github-project-automation
Bot
moved this from Ready for review
to Done
in Animation Team Issue Triage
AThousandShips
added
archived
and removed
cherrypick:4.3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #103070
I didn't use a floating point comparison here because it should match exactly. Also let me know if there would be any unintended side effects. Animation stuff is not my strong suit.