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

Return a sensible value for animation component duration if no animation is playing #4765

Merged
merged 6 commits into from
Oct 19, 2022

Conversation

yaustar
Copy link
Contributor

@yaustar yaustar commented Oct 19, 2022

Stops exception being thrown when using duration if there is no animation playing

Returns Number.MAX_VALUE instead of 0 as some developers may be dividing by this number for a normalised value and this avoids divide by 0.

A bit torn on whether this is the 'right' number to return though?

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@yaustar yaustar added bug area: animation Animation related issue labels Oct 19, 2022
@yaustar yaustar requested a review from a team October 19, 2022 14:37
@yaustar yaustar self-assigned this Oct 19, 2022
@yaustar yaustar marked this pull request as draft October 19, 2022 14:38
@yaustar yaustar marked this pull request as ready for review October 19, 2022 14:39
@mvaligursky
Copy link
Contributor

I'd like returning 0 more I think, and using Debug.warn to let the user know.

@yaustar
Copy link
Contributor Author

yaustar commented Oct 19, 2022

Yeah, I'm very torn on getting infinity errors 'silently' and not seeing it until it hits something that fires an exception (usually far away in the call stack from the actual cause) and actually giving a number that makes sense common use cases.

If you still think it should be 0, I will change it but have the above concern.

@mvaligursky
Copy link
Contributor

I understand, but a warning should solve the problem really.

@yaustar
Copy link
Contributor Author

yaustar commented Oct 19, 2022

Updated PR to use 0 with warning

Co-authored-by: Martin Valigursky <59932779+mvaligursky@users.noreply.github.com>
@yaustar yaustar merged commit 8077ae5 into main Oct 19, 2022
@yaustar yaustar deleted the animation-return-0-if-no-anim branch October 19, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: animation Animation related issue bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants