Add support for explicit control over model animations #9339

markw65 · 2021-01-25T00:54:47Z

Cesium currently only supports time based animation. This can be inconvenient if the phase of the animation is related to something other than time (eg distance along a path of an object moving at a variable speed).

This came up before in a pull request here, but the author was persuaded that it was better to use nodeTransformations to explicitly control the model. That was (just) doable with that example, because there were just 3 pairs of wheels, all of which needed the exact same, relatively trivial, transformations.

My model has a bicycle chain with 90+ links, two wheels, two chain rings (with different rates of rotation, which are both different again from the rate of rotation of the wheels), two drive arms, and a rider. Using nodeTransformations would be extremely complex, and any changes to the model would require updating all the associated javascript. Also, on the site where I use the model, I actually use different models for different types of activity - so I want to just be able to drop in a new model, and have it "just work" (to make it work, I have to ensure that the animation's "duration" in seconds is equal to the distance in meters the model would move during one iteration of the animation).

This adds an animationTime property to ModelAnimation. If set, it's used by ModelAnimationCollection.update to determine the localAnimationTime, rather than using the current clock time.

I also added a manualAnimation property to ModelAnimationCollection so we can still do the short circuit exit from ModelAnimationCollection.update.

The new sandcastle example is just a clone of Time Dynamic Wheels, rewritten to use a more complex model, and the new functionality.

cesium-concierge · 2021-01-25T00:54:49Z

Thank you so much for the pull request @markw65! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

❌ Missing CONTRIBUTORS.md entry.
- Please add yourself to the contributors file!
✔️ 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.

markw65 · 2021-01-25T17:26:44Z

@mramato @hpinkos You reviewed #7361 which originally had a similar intent (although a different approach). Any chance you could take a look? If it is something worth pursuing, any directions on how to write tests for this would be appreciated.

markw65 · 2021-02-09T20:09:06Z

@mramato @hpinkos I think this is fully ready for review; I found the existing tests, and modified one to test the new features. I've also updated CONTRIBUTERS.md and CHANGES.md, but don't know how to get cesium-concierge to notice.

markw65 · 2021-02-10T00:58:31Z

@OmarShehata maybe you could take a look?

cesium-concierge · 2021-03-12T03:00:08Z

Thanks again for your contribution @markw65!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge · 2021-06-10T02:00:11Z

Thanks again for your contribution @markw65!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge · 2021-09-08T02:00:20Z

Thanks again for your contribution @markw65!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

markw65 · 2021-10-19T23:32:26Z

@ebogo1 any chance you could look at this, or find someone who could look at it? I've tried pinging people who seemed likely to be interested, and posting in the forums (https://community.cesium.com/t/position-based-control-over-animations/11558 and then https://community.cesium.com/t/how-do-i-champion-my-pull-request/12286), but no response.

ebogo1 · 2021-10-21T22:49:06Z

@markw65 apologies for the delay - I'm currently trying to get a few things wrapped up for the upcoming release but I'll try to get back to this as soon as possible. Thanks!

cesium-concierge · 2022-01-20T03:00:23Z

Thanks again for your contribution @markw65!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge · 2022-04-20T02:00:12Z

Thanks again for your contribution @markw65!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

markw65 · 2022-04-21T14:33:05Z

I'm still very interested in getting this merged. I simply have no idea how to "champion" it. Nobody responds to anything I say about it. I'll rebase again, just in case...

ebogo1 · 2022-04-22T21:19:08Z

Hi @markw65 - apologies for the long wait and thanks for keeping the PR up to date.

I think adding some kind of manual control over "local time" is a fair extension for animations but the current implementation seems a bit restrictive to me. I could be misunderstanding, but to me it looks like the design around the new animationTime property assumes that it'll be used with multiple models updating it similarly (i.e. with the sampled distance property in your new Sandcastle). I wonder if there's a more generalized solution to control the animation time deltas - maybe it'd require a slightly different approach in ModelAnimationCollection.

j9liu · 2022-04-22T21:29:01Z

Hi @markw65,

I noticed this while working on bringing animations to ModelExperimental in #10314. It'd be useful to give users more control over animations!

I feel like the implementation can be reworked somehow. It would be nice to have animations be independent of the scene frame's time, that's for sure. But, at least to me, it doesn't feel right that the user would have to manually increment some value and directly set it in the animation every frame. I'm not sure what would be better API design here, to be honest.

My thoughts aren't very refined, but this is what I'm thinking:

having some flag to allow animation independent of scene time is not a bad idea, though maybe rename from manualAnimation to something else
multiplier is currently a readonly property -- what would happen if we allowed the user to set this dynamically?
if the above suggestion doesn't work, let's rework animationTime. Instead of animationTime being a number that is changed, we could make it a property that's some sort of callback. The default would involve the current time (in real life), so that the animation can play even if the scene time pauses, the animation will still continue. But we can also allow this to be overridden by the user.

These are just thoughts, not concrete suggestions, so don't take this as a call to implement them right away. I think having input from other community members and developers would help us figure out where to go from here.

markw65 · 2022-04-23T01:32:59Z

Thanks for the feedback!

@ebogo1

the design around the new animationTime property assumes that it'll be used with multiple models updating it similarly

I don't really follow? If there are multiple models, they will presumably be moving separately, and will consequently need to have their animationTimes set separately?

eg this example

@j9liu

I'm not attached to the naming scheme in any way
multiplier really isn't a good way to go - and this was discussed in Sandcastle example for model with spinning wheels #7295, which initially suggested using multiplier for a similar reason. It doesn't work well because you want (eg) the amount the wheels turn by to be proportional to the distance covered by the model. In theory you could get that by modifying multiplier, but in practice, with varying speeds, its not easy. In addition, the implementation of multiplier is such that any time you change it, you effectively reset the phase of the animation, so every alteration to multiplier causes a skip in the animation (at least, thats what I remember from following Sandcastle example for model with spinning wheels #7295).
making animationTime a callback which defaults to mimicking the current behavior sounds reasonable. Let me investigate. And if @ebogo1 has any feedback on that suggestion I'd appreciate it...

ebogo1 · 2022-04-25T14:13:53Z

I like the idea of turning the new animationTime property into something more generic. In this example a sampled distance property works but I think if we're adding more control over the animation system we should keep in mind more generalized use cases. I do agree with @j9liu that setting animationTime each tick feels a bit clunky - ideally a callback function that has a generic interface for computing its own local time would be able to handle all sorts of animations.

As a note, I believe Three.js and Babylon.js both expose a time multiplier as the main way to control animation speed.

markw65 · 2022-04-27T20:23:35Z

As a note, I believe Three.js and Babylon.js both expose a time multiplier as the main way to control animation speed.

So I agree that multiplier is useful - I'm even using it in the project that uses this patch. Essentially, I use it as a scale factor so that each model would move by durationSeconds meters across its full animation cycle, assuming no slipping of wheels.

My claim is that if you want non-time based control over the animation, then multiplier is a very awkward way to go; and the current implementation of multiplier simply doesn't work because every time you change it, it causes a jump in where the animation is.

eg suppose multiplier is 1, and you're halfway through the animation cycle, and then you change multiplier to 2 (while only allowing time to advance by an insignificant epsilon). Then suddenly, you're at the end of the animation cycle (this assumes you update _duration at the same time as multiplier; much worse things happen if you don't). You could obviously fix that by also maintaining an _offset, and updating that every time multiplier changes. But its still not a convenient way to map an arbitrary function onto time...

[Edit] I've thought about this more, and I think I can explain my objection to multiplier much more clearly.

The multiplier approach is trying to recreate a function by integrating its derivative. The problem with that is that this will always introduce an "arbitrary constant" (which manifests as _offset above).

To make this concrete, take my model. After it has rolled a certain distance, there is a specific point in its animation cycle that it should be at. So eg if it covers 10m per animation cycle, after going 45m, it should be exactly half way through. If you run the clock from the start, and set multiplier appropriately at each tick (and have some mechanism that adjusts _offset as described above to prevent jumps) you can just about arrange for that to happen. But what if the user just uses the timeline to jump to a particular time? There is no way to set multiplier alone and get the correct offset.

In other words, multiplier is an ok way to control the animation incrementally along the path, but no good at all if you're jumping around.

markw65 · 2022-04-27T20:35:05Z

@j9liu

The default would involve the current time (in real life), so that the animation can play even if the scene time pauses, the animation will still continue.

Just wanted to clarify, because as I understand it, thats not the current behavior. It uses the simulator time, not real time.

j9liu · 2022-04-27T22:33:07Z

@markw65 -- You're right, I misspoke. Thanks for the catch!

markw65 · 2022-04-28T02:15:58Z

Ok, I've updated the branch. I've added it as a patch on top of the old one, but I think it's probably better to just flatten it; its got more in common with the original code than with my first attempt. Let me know what you'd prefer.

I got rid of manualAnimation, and replaced it with animateWhilePaused. The basic reason for the flag is the same; when its false, we can skip the entire update loop when the scene time hasn't changed (as the current code does). But this time, you only need to set it if you want to provide an animation function that animates while scene time is paused. So unlike the manualAnimation flag, most uses of animationTime won't need to set it (so eg my own use case, although its distance, rather than time based, doesn't need the flag because the distance can't change unless the time does).

I'm not too familiar with the comment based typing scheme; but I think I eventually got it right(?), because my own project is written in typescript, and kept complaining until I got all the comments in place...

markw65 · 2022-04-28T16:25:10Z

I think the build failure was a travis glitch (makeZipFile seems to have timed out). I've rebased the branch to get another build going.

markw65 · 2022-04-28T16:49:16Z

No, it was a prettier check on my sandcastle example. Should be fixed now.

j9liu

Hi @markw65,

Thanks for the quick updates! I wasn't able to completely review the new changes, but I wanted to give it at least a quick look. Here's a couple suggestions regarding documentation / code style; next week we can take a closer look at the code to make sure it's robust.

Source/Scene/ModelAnimationCollection.js