Animations by cdata · Pull Request #421 · google/model-viewer

cdata · 2019-03-08T16:14:37Z

Animations

This change proposes the addition of new API (and related bug fixes) to configure and present animated glTFs with <model-viewer>. Morph, skeletal and keyframe should all be supported.

New public API introduced by this change:

Attribute	Default value	Description
`autoplay`	`false`	If true, animations will play. Otherwise, they will be stopped. Playing animations will be reset if `animated` changes to false.
`animation-name`	`undefined`	If set, `<model-viewer>` will look for an animation with this name when played. If an animation with that name is found, plays that animation. Otherwise, if an animation with that name is not found, falls back to the first animation (if any).
`animation-crossfade-duration`	`300`	The duration of the crossfade between animations when changing them via `animation-name`.

Property	Default value	Description
`availableAnimations`	`[]`	After a model finishes loading, if that model has any animations, they will be listed by string name in this property's value.
`paused`	`true`	Readonly, reflects the play/pause state of the `<model-viewer>`. A `<model-viewer>` displaying a model with no animations is always considered paused.
`currentTime`	`0`	This read/write property represents the current track time (in seconds) of the currently playing animation (if any). If a value is assigned, the animation will seek to the time specified.

Method	Description
`play()`	Plays the animation (selected by `animation-name`, or else falling back to the first animation) if it is available.
`pause()`	Pauses any currently playing animation

Event	Description
`pause`	Dispatched when the `<model-viewer>` is paused. The implicit default state of `<model-viewer>` is paused, and a `pause` event will not be dispatched until the `<model-viewer>` has been played at least once.
`play`	Dispatched when the `<model-viewer>` begins playing animations.
`poster-visibility`	This event is fired when the visibility of the poster image changes. The current visibility state can be read from the `event.detail.visible` property

Additional highlights

Various bugfixes and associated tests to handle race conditions related to changing the src attribute quickly
A new section has been added to our documentation site: "Animations"
Poster image positioning has been changed so that the image is centered within the bounding rect of the <model-viewer>
A patch that I wrote a while back to correctly clone parsed glTFs w/ animations has been updated and integrated here. This will be necessary until geometry.clone() doesn't copy reference to bones/skinweights/animations etc. mrdoob/three.js#5878 and No way of cloning a mesh so it works with a skinning material mrdoob/three.js#11574 are addressed in Three.js
Test timeouts have been increased to 45s because iOS and IE11 are buckling under the strain at this point. Filed Test suite is experiencing large numbers of timeouts #433 to track.

Examples

`autoplay`	`animation-name="Run"`	Switching `animationName` on an interval

Animated model, in paused state	Switch `animation-name` while paused

Future work

Configurable iteration count: Configurable animation iteration count #424
Built-in default, customizable UI: Built-in, optional UI for end-user control over animations #425

Fixes #404
Fixes #371

googlebot · 2019-03-08T16:14:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

src/three-components/CachingGLTFLoader.js

jsantell

😍 great work!

jsantell · 2019-03-14T18:33:58Z

examples/assets/attributions.md

+licensed under <a href="https://creativecommons.org/publicdomain/zero/1.0/">CC0</a>
+(<a href="https://github.com/mrdoob/three.js/tree/dev/examples/models/gltf/RobotExpressive">source</a>)
+
 ## small_hangar_01


Horse.glb needs attribution

Attribution added

jsantell · 2019-03-14T18:36:21Z

index.html

            <p>Configures the model with custom text that will be used to describe the model to viewers who use a screen reader or otherwise depend on additional semantic context to understand what they are viewing.</p>
          </li>
+          <li>
+            <h4>animated</h4>


I think animate might be a better attribute name than animated (present vs past tense), although can't think of good prior art (other than "checked", which is past tense, but seems like a different case?)

Ugh, I forgot: I originally used animate, but it's a built-in method of HTMLElement (part of the Web Animations API).

(and yah that's definitely not going to be confusing for folks at all)

😩

This has been left as is, but if there is another name we like that doesn't overlap with a built-in API, I am open to changing it.

jsantell · 2019-03-14T18:37:29Z

src/features/animation.ts

+          class AnimationModelViewerElement extends ModelViewerElement {
+            @property({type: Boolean}) animated: boolean = false;
+            @property({type: Boolean, attribute: 'pause-animation'})
+            pauseAnimation: boolean = false;


pause-animation seems unused/undocumented (other than in the scenario where it's checked with !animated), is this necessary?

Instead of pause-animation, there does seem like there would be a need to reset back to the default/T-Pose, so there is a difference between 'pausing' the animation (freezing in the middle of an animation) as opposed to 'removing' the animation -- of course, I don't think it necessary to solve this in this PR

Yes, it is different from stopping the animation. Pause animation ~~is documented, and~~ will be necessary in order to implement @yuinchien 's mocks to spec in #425

I think the effect you are describing is achieved when setting animate to false. Did you have something else in mind?

pause-animation was not adequately documented, sorry. I added docs for it.

@cdata yes, with pause-animation and !animate, both 'freeze' and 'reset' flows are supported

After API refactor, to "pause" you must invoke pause() (there is no corresponding attribute/property). And, to "stop" you subsequently set .currentTime = 0.

jsantell · 2019-03-14T18:39:06Z

src/features/animation.ts

+             * model. The promise rejects if the currently loaded model is
+             * changed to something new due to the src attribute changing.
+             */
+            [$updateModelLoadsPromise]() {


Environments has a similar need/way of doing this -- wonder if there's a way to generalize this across all features?

After offline discussion, I'm looking into whether this is necessary or not.

I reduced the complexity of this implementation substantially, and I think it should be fine. Thanks for calling this out!

jsantell · 2019-03-14T18:44:22Z

src/model-viewer-base.js

+    // NOTE(cdata): If a property changes from values A -> B -> A in the space
+    // of a microtask, LitElement/UpdatingElement will notify of a change even
+    // though the value has effectively not changed, so we need to check to make
+    // sure that the value has actually changed before changing the loaded flag.


This seems like it'd be valuable for all properties, not just src

Possibly, although the consequences of src change side-effects occurring unnecessarily seem particularly severe as they could affect all other features.

jsantell · 2019-03-14T18:48:34Z

src/features/loading.js

    get loaded() {
-      return super.loaded || this[$preloaded];
+      return super.loaded ||
+          (this.src && CachingGLTFLoader.hasFinishedLoading(this.src));


This method, while probably valuable for tests at least, seems that it'd be more appropriate to live on this.src, our Model class (which could just wrap this); CachingGLTFLoader is an implementation detail that would benefit from continuing being hidden inside of Model, or at least out of features/*

I don't know that I agree. CachingGLTFLoader was in fact the abstraction that enabled preloading to work in parallel with normal loading via setting src. Please offer a more concrete proposal for how you would change this!

Keeping this as-is, but remain open to suggestions. I don't think it's the right answer to conflate preloading and the implementation in the Model class, because untangling that is what begot CachingGLTFLoader in the first place.

jsantell · 2019-03-14T18:49:38Z

src/three-components/CachingGLTFLoader.js

+          .catch(() => {});  // Silently ignore exceptions here, they should be
+                             // caught by the invoking function
+      cache.set(url, loadAttempt);
+      // window.loadAttempt = loadAttempt;


nit: comments

jsantell · 2019-03-14T18:51:37Z

src/features/loading.js

+        if (preloaded) {
+          this.dispatchEvent(new CustomEvent('preload', {detail}));
+        } else {
+          loader.preload(this.src)


nit: prefer await

I'll re-evaluate if this depends on asynchronous code flow

I factored this out so that it's wrapped in an async function.

jsantell · 2019-03-14T18:51:53Z

src/three-components/CachingGLTFLoader.js

+          .then(() => {
+            preloaded.set(url, true);
+          })
+          .catch(() => {});  // Silently ignore exceptions here, they should be


Any reason using .then instead of await? This will swallow exceptions as indicated, but how would invoking function catch that if they're not thrown?

When you invoke then or catch in a promise, it does not modify the state of a promise in place. The methods return new promise instances that are resolved differently depending on how you invoked them.

We don't cache the "caught" promise. So, try/catch blocks outside of this scope will still work as expected.

And yeah, the reason for using then is that the asynchronous behavior is preferred.

As mentioned in #421 (comment) I'll re-evaluate if this is necessary.

Rewritten with await.

jsantell · 2019-03-14T18:54:00Z

src/three-components/Model.js

-    const scene = await this.loader.load(url);
+    // If we have pending work due to a previous source change in progress,
+    // cancel it so that we do not incur a race condition:
+    if (this[$cancelPendingSourceChange] != null) {


Similar concept to other features ensuring that the changes are still valid; can that be abstracted so that model can handle it everywhere? No good ideas currently, but this seems like a potential growing source of race conditions throughout features

I disagree, this problem is fairly unique to the consequences of changing the src. Let's discuss offline.

After reviewing, I remain convinced that changing the src requires special considerations. I'm open to concrete suggestions for refactoring, though.

cdata · 2019-03-15T14:16:49Z

Tests ended in IE11 timeout, flagging for #433

mrdoob · 2019-03-15T15:26:14Z

What about autoplay instead of animated? Mimicking audio and video tags.

cdata · 2019-03-15T15:35:27Z

autoplay is a good idea, I like that.

Although conceptually, we would probably have to introduce an explicit API for pausing + resetting the animation.

For example, to "stop" a <video> you have to do something along the lines of:

video.pause();
video.currentTime = 0;

WDYT? Should we go for something more like that?

I'm also interested to hear what others think of autoplay as a name @jsantell @smalls

smalls · 2019-03-15T16:51:34Z

I'm good with autoplay - in some ways it's not as descriptive (there's no association with an animation), but I think the developer intent is clearer (with animated, for instance, it's not clear if it should be animated our could be animated).

Chris and I chatted about pause() && currentTime=0 vs stop(); I'm fine with the former, and I think that'll give developers extra flexibility. We can always add stop() on top of that as a convenience method if needed.

Overall using <video> as inspiration for the API makes sense - they've probably thought through some of these issues already.

cdata · 2019-03-15T17:16:18Z

This all SGTM. I'm going to change the current proposal so that the API tracks closely to <video>. The bill of changes looks like this in my mind, so please call out if you think any of these changes aren't right:

animated will become autoplay
A play method will be added, and associated play event dispatched when appropriate
A pause method and a paused property will be added, and associated pause event dispatched when appropriate
- pause-animations attribute/property will be removed
currentTime will be exposed as a read/write property
animation-name, availableAnimations and animation-crossfade-duration will remain as-is

cdata · 2019-03-18T16:08:14Z

API has been overhauled to be more like <video> (PR description updated)
Additional tests / demo added
Docs updated to reflect API change

PTAL at your leisure!

cdata · 2019-03-18T22:50:56Z

src/three-components/CachingGLTFLoader.js

+      // preloaded.set(url, true);
+      //})
+      //.catch(() => {});  // Silently ignore exceptions here, they should be
+      //// caught by the invoking function


Need to remove

smalls

Thanks, the API's looking great!

mrdoob · 2019-03-19T23:53:08Z

🎉🎉🎉

sowmyy · 2020-06-12T11:23:13Z

Is there a way to import the array from the gltf file and use it in the component? Because, for me only one of the multiple animations play. rest doesn't! Is this bug already being resolved or yet to ? Also, which is the recommended format for rendering animated 3D models - GLTF or GLB ?? @cdata

mrdoob mentioned this pull request Mar 8, 2019

(WIP) Animation support. #395

Closed

mrdoob reviewed Mar 8, 2019

View reviewed changes

src/three-components/CachingGLTFLoader.js Show resolved Hide resolved

This was referenced Mar 12, 2019

Configurable animation iteration count #424

Closed

Built-in, optional UI for end-user control over animations #425

Closed

cdata force-pushed the animations branch 3 times, most recently from 454cbbf to f5559bf Compare March 14, 2019 05:09

cdata changed the title ~~(WIP) Animations~~ Animations Mar 14, 2019

cdata marked this pull request as ready for review March 14, 2019 05:52

cdata requested review from jsantell, mrdoob and smalls March 14, 2019 18:30

jsantell previously approved these changes Mar 14, 2019

View reviewed changes

cdata dismissed jsantell’s stale review via 54d2325 March 15, 2019 04:31

mrdoob and others added 6 commits March 15, 2019 15:06

Added morph animations support.

baf0f06

Add animations feature, skeletal animations fix

e3ffcb9

Refactor preloading to support cancellation

62d117f

Add docs, tests, license, attributions

9202ba0

Changes per review feedback

751cac2

Change API to be more like <video> element

1c47865

cdata force-pushed the animations branch from 54d2325 to 1c47865 Compare March 17, 2019 16:26

cdata requested a review from jsantell March 18, 2019 16:08

cdata commented Mar 18, 2019

View reviewed changes

smalls previously approved these changes Mar 19, 2019

View reviewed changes

Remove unnecessary comments

9bae8ee

cdata dismissed smalls’s stale review via 9bae8ee March 19, 2019 22:33

cdata merged commit 86f704a into master Mar 19, 2019

cdata deleted the animations branch March 19, 2019 22:34

cdata mentioned this pull request Mar 22, 2019

Materials not being cloned correctly #440

Closed

Conversation

cdata commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Animations

Additional highlights

Examples

Future work

Uh oh!

googlebot commented Mar 8, 2019

Uh oh!

Uh oh!

jsantell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdata Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdata Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdata commented Mar 8, 2019 •

edited

Loading

cdata Mar 14, 2019 •

edited

Loading

cdata Mar 14, 2019 •

edited

Loading

cdata commented Mar 15, 2019 •

edited

Loading

cdata commented Mar 15, 2019 •

edited

Loading