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

Clamp non-looped animations to start and stop times. #8769

Merged
merged 4 commits into from
Apr 23, 2020
Merged

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Apr 18, 2020

Fixes #7387.

If Cesium's current time went outside of a non-looping glTF animation time interval, Cesium would not perform one final update to push the animation as far as it could go in the direction of that time.

Here's a Sandcastle demo on master that tries to demonstrate the problem. The green area on the timeline shows where the animation is playing. If you jump the time into one of the outside areas (yellow or red), the cube won't update, it will remain frozen in some in-between state.

anim-issue-v2

Here's the same Sandcastle demo on this branch.

@cesium-concierge
Copy link

Thanks for the pull request @emackey!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@emackey
Copy link
Contributor Author

emackey commented Apr 18, 2020

I can't see what Travis is doing, but there's probably a failed test on here, I'll have to look into it.

@emackey emackey marked this pull request as draft April 18, 2020 21:23
@mramato
Copy link
Contributor

mramato commented Apr 20, 2020

I can't see what Travis is doing

Why not? You should be able to get the full log. Here you go either way.

  Scene/Model
    ✗ raises animation start, update, and stop events when removeOnStop is true
	Expected 5 to equal 4.
	<Jasmine>
	@Specs/Scene/ModelSpec.js:1776:41
	fulfilled/p<@Source/ThirdParty/when.js:194:34
	_then/<@Source/ThirdParty/when.js:295:13
	processQueue@Source/ThirdParty/when.js:645:11
	_resolve@Source/ThirdParty/when.js:331:16
	promiseResolve@Source/ThirdParty/when.js:357:11
	poller@Specs/pollToPromise.js:26:16
    ✗ Animates with an explicit stopTime
	Expected 3 to equal 2.
	<Jasmine>
	@Specs/Scene/ModelSpec.js:1846:39
	customizeJasmine/window.it/<@Specs/customizeJasmine.js:40:22
	<Jasmine>

@emackey emackey marked this pull request as ready for review April 20, 2020 19:35
@emackey
Copy link
Contributor Author

emackey commented Apr 20, 2020

OK, tests uncovered another edge case, where user's stop time happens before the animation's own stop time. This is fixed now, and new tests explicitly cover the edge cases I know about. Coverage remains at 100% for this section of code.

@emackey
Copy link
Contributor Author

emackey commented Apr 22, 2020

This is ready.

@mramato
Copy link
Contributor

mramato commented Apr 23, 2020

Looks simple and straightforward to me and can confirm the fix works as intended. @lilleyse did you want to take a look at this?

@lilleyse
Copy link
Contributor

Looks good to me

@lilleyse lilleyse merged commit 4ca9e1b into master Apr 23, 2020
@lilleyse lilleyse deleted the anim-start-stop branch April 23, 2020 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gltf] Missing last frame during animation?
4 participants