-
Notifications
You must be signed in to change notification settings - Fork 809
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
Model animate interaction prompt #788
Model animate interaction prompt #788
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to feedback about the UX here, especially if you've been testing some of these options.
src/features/controls.ts
Outdated
const promptCenter = promptLeft + (promptWidth / 2); | ||
const promptOffset = promptCenter - modelViewerCenter; | ||
|
||
(this as any)[$scene].pivot.rotation.y = promptOffset * OFFSET_ROTATION_MULTIPLIER; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like promptOffset is in units of pixels and that it moves by a fixed number of pixels. I wonder if it wouldn't be better to have the motion scale to a % of the canvas size, but keep the angular rotation constant? What I notice now is that the prompt motion relative to the model changes based on how I adjust the window size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update to a relative percentage 👍
transform: translateX(-5%); | ||
} | ||
75% { | ||
transform: translateX(5%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we change this animation to be intermittent? I think it would be more pleasing to the eye if it were a bit shorter with a few seconds of space in between that didn't show the SVG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with the intermittent animations, after a bit of searching it doesn't seem there's a way to pause animation between (infinite) iterations through CSS.
There's a few alternatives:
- placing a pause within the animation keyframes
- performing a few iterations of the animation each with a delay on them
In terms of the UX, what do you think about this animation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty good, though are we really getting anything out of the symmetry? I might just rotate right and back to center and call it good. I think the timing is quite good, and this will allow it to move a bit slower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a preview of what we were thinking about the fine tuned adjustments for the animation: https://translucent-lunch.glitch.me
Updated so that it sticks to the each side a little longer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elalish We're feeling pretty good about the latest animation that @maaslalani just posted.
The symmetry is really important to us as it shows a good range of motion, and makes for a good flow.
FYI: In a subsequent PR we also have some changes to the hand svg based on some usability research we did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That animation looks really good to me! Thanks for the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pushmatrix if we changed the animation in a subsequent change (perhaps the timing function, or the duration), would you consider that a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we would.
src/template.ts
Outdated
animation-name: wiggle; | ||
animation-duration: 3s; | ||
animation-iteration-count: infinite; | ||
animation-timing-function: linear; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to use a different function here to make the direction change less sudden?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, will switch to ease-in-out
.
src/features/controls.ts
Outdated
@@ -68,12 +68,16 @@ const PHI = 2.0 * Math.PI; | |||
const AZIMUTHAL_QUADRANT_LABELS = ['front', 'right', 'back', 'left']; | |||
const POLAR_TRIENT_LABELS = ['upper-', '', 'lower-']; | |||
|
|||
const OFFSET_ROTATION_MULTIPLIER = 0.005; | |||
|
|||
export const DEFAULT_INTERACTION_PROMPT_THRESHOLD = 3000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a small bug where the model seems to jerk when the prompt first comes up. I believe this is because the model starts to auto-rotate just before the prompt comes up, and then the prompt resets the rotation to zero, which could get worse if this threshold attribute is increased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think the best way to handle auto-rotate is?
We could:
- not play the auto-rotate animation if the interaction prompt is going to be displayed.
- have the auto-rotate animation play as normal and (smoothly) animate the model back to the centre before the interaction prompt animation begins.
- animate the interaction prompt and model at whatever rotation it happens to be at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps an even simpler answer? How about make the auto-rotation delay (which is not currently configurable) equal to the interaction prompt threshold plus enough delay to ensure the prompt always fires first. @cdata Does this sound good to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, I would like to state plainly that I increasingly desire for us to deprecate auto-rotate
in favor of #738 because the way it works is kind of annoying and I think everyone would be happier without it.
Imagining the user intention of someone literally writing this HTML:
<model-viewer auto-rotate></model-viewer>
They have explicitly asked for auto-rotation, regardless of interaction-prompt behavior. I think bullet #3 from @maaslalani is what I expect in that case. Auto-rotation should happen as normal, and the wiggle position should be additive to whatever the turntable rotation value happens to be in a given frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A potentially easier way to achieve bullet #3 would be to animate the camera instead of the pivot, since this way they would be independent and their effects would add automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a breaking change, and also make auto-rotate
even more annoying and useless (because it's just sugar over camera-orbit
at that point). Orbiting the camera != rotating the pivot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have misunderstood @elalish . I now understand that he was referring to the interaction prompt, not auto-rotate
.
So, yeah, I agree, that would be a reasonable path forward as well.
One thing we need to check for is what happens when you put in a custom interaction prompt via a |
|
||
const modelViewerCenter = modelViewerLeft + (modelViewerWidth / 2); | ||
const promptCenter = promptLeft + (promptWidth / 2); | ||
const promptOffsetRelative = (promptCenter - modelViewerCenter) / modelViewerWidth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to guard against when modelViewerWidth = 0 to avoid divide by zero errors.
This can happen if the model viewer starts hidden on a page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Fixed 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool work @maaslalani 🙌
Let's figure out how to work the necessary configurability into this change!
src/features/controls.ts
Outdated
@@ -143,7 +146,10 @@ export const ControlsMixin = <T extends Constructor<ModelViewerElementBase>>( | |||
|
|||
protected[$promptElement] = | |||
this.shadowRoot!.querySelector('.controls-prompt')!; | |||
protected[$promptElementSVG] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This element is going to be removed from the render tree (and not animated) when a user slots a different element into the controls-prompt
slot, for example:
<model-viewer>
<img src="custom-prompt.png" slot="controls-prompt">
</model-viewer>
In this case, you would need to query the slot for its assigned node, because the built in SVG is just a fallback that is only displayed when no other nodes are assigned to <slot name="controls-prompt">
.
Is there a way we could rework this so that we don't need a direct reference to the node that is inside the slot? That would save us from complicated dual-code-paths to handle slotted vs fallback nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed so that we no longer depend on the SVG element (c7dfc33), Since the opacity is being animated on the slot itself removing the class would suddenly fade out the prompt element. I added a div.wrapper
to solve this. If there's a better way to do this let me know! Tested out with an img
and works correctly, so the icon is customizable via slots.
src/features/controls.ts
Outdated
const { | ||
left: modelViewerLeft, | ||
width: modelViewerWidth, | ||
} = this.getBoundingClientRect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's potentially expensive to compute this on a rAF. We actually measure the current dimensions of the element every time it resizes:
Would it be possible to rely on those values instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -165,6 +165,27 @@ template.innerHTML = ` | |||
transform: translateY(-100%); | |||
} | |||
|
|||
@keyframes wiggle { | |||
10%, 12% { | |||
transform: translateX(-5%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make this animation configurable, and in particular we need to enable users to opt-out of it. In a perfect world, we would simply expose the interaction prompt slot as a CSS Part, and then someone could write CSS from the outside that looks like this:
@keyframes custom-animation {
/* whatever you want */
}
model-viewer::part(controls-prompt) {
animation-name: custom-animation;
/* etc. */
}
We should totally do the ::part
thing no matter what. It will only work in Chrome, but it will demonstrate the principle of how we wish things would work everywhere. All it takes is adding a part="controls-prompt"
attribute to the <slot name="controls-prompt">
element. Since it's just a matter of adding an attribute to an HTML element, it will gracefully degrade to doing nothing at all in browsers that do not support ::part(...)
.
Sadly, we live in the world where ::part(...)
only works in Chrome. Although there are polyfills, they kind of suck and require JavaScript cooperation. So, we need to consider feasible alternatives...
One potential alternative would be to make the properties being animated configurable with CSS Custom Properties. For example, we can tweak the wiggle
animation to look like this:
@keyframes wiggle {
10%, 12% {
transform: var(--controls-prompt-initial-transform, translateX(-5%));
}
30%, 32% {
transform: var(--controls-prompt-final-transform, translatX(5%));
}
}
This would allow someone to tweak the beginning and end transform
property values of the wiggle animation from outside the element. They could create a specialized kind of motion, or disable the animation entirely by making the beginning and end values the same. We could do something similar for the fade
animation as well, so that the opacity level can be controlled in a similar way.
The multiple custom property route is a somewhat cumbersome configuration strategy in cases like this. Users will have to set the values for multiple custom properties at once to achieve a custom effect, and the only way to know the property names they should set is to read our docs. And, the types of styles they can change will be limited compared to the ::part(...)
approach.
As an alternative, we could add an attribute that allows folks to switch back to the previous, relatively basic interaction prompt behavior. For example, we could allow for interaction-prompt-style="basic"
, which will simply set animation-name: none
on the <slot name="controls-prompt">
element. interaction-prompt-style="wiggle"
would be the default.
Yet another alternative would be to offer a single CSS custom property to override the animation-name
property for the <slot name="controls-prompt">
element. The sole purpose of this custom property would be to give users the ability to disable the animation and thus prevent the model from wiggling. This is perhaps the most crude way to enable an opt-out, but it is probably also the easiest to implement.
After considering all three of these options, I think I'm inclined to go with the attribute approach. What do y'all think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building off the 3rd option, and offering a CSS custom property to override the animation-name
property. Instead of animation-name
, we can simply add the override to all the animation properties through the animation
shorthand. This way the user can specify their own animation keyframes (name), animation duration, delay, iteration count, etc... by overriding this exposed property.
CSS Syntax:
animation: name duration timing-function delay iteration-count direction fill-mode play-state;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maaslalani it's a nice idea, but unfortunately shadow root encapsulation will not allow for this. If I specify a keyframe animation in the global document scope, it won't be available in the local shadow scope. The user would be able to change the name that is used, but CSS won't be able to find the keyframes that correspond to that name (except in legacy polyfill scenarios, where encapsulation is just a facade). Here is an example that demonstrates the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose there is precedence for the attribute approach. We have the whole allow-when-focused
and always-allow
which are opinionated ways of interacting with the viewer.
So it comes with basic
and wiggle
out of the box, but it would be nice to be able to add your own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it comes with basic and wiggle out of the box, but it would be nice to be able to add your own.
@pushmatrix agreed, and this is why this change is relatively difficult to land as-is. The UX is quite opinionated and difficult to customize. Ideally we would lean in to something like #738 to accomplish specialty interaction prompts like this.
As mentioned above, CSS parts would make this effect a lot easier to customize. For now, we'll have to wait for future web platform features to land, and try to get to #738 as quickly as we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WebKit folks have blessed us with some promising news on this topic: https://webkit.org/blog/9609/release-notes-for-safari-technology-preview-94/
Added support for the ::part() pseudo element from CSS Shadow Parts
transform: translateX(-5%); | ||
} | ||
75% { | ||
transform: translateX(5%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pushmatrix if we changed the animation in a subsequent change (perhaps the timing function, or the duration), would you consider that a breaking change?
@maaslalani great work on the updates! In the interest of time, I'm going to make a proposal for some final tweaks to this PR. My hope is that we can land it ASAP and cut a release on Wednesday or Thursday. |
We had an accessibility audit done, and the main result we found with this (and the previous CTA) is that continuous animations (even with a pause) can be problematic. This is for someone with a cognitive disability, who's easily distracted, or someone having difficulty reaching and interacting with the model in order to cease the animation completely. It was suggested a maximum of 3 rotations and then stopping. I'd be tempted to make that a parameter, but then it feels like we have parameter bloat. Curious about your thoughts on this. |
@pushmatrix even on basic UX grounds, I find the interaction prompt (both iterations) to be somewhat cloying when it keeps going and never stops. I think an Another approach might be something like For the time being, since we have an attribute to turn off the prompt, I'm inclined to suggest that any limit on its display be an implementation detail for Shopify. Regardless, please file an issue so that we can make this more ergonomic and accessible! |
These changes were landed via #834 . Thanks again @maaslalani ! |
Reference Issue
Fixes: #708
Removes SVG animations from interaction-prompt and uses a CSS based animation for the interaction prompt and slides it back and forth, if the interaction prompt is visible then the position of the prompt is monitored and the model rotates accordingly.
General Approach
The approach was to use CSS keyframe animations to move the interaction prompt cursor and set the
pivot.rotation.y
based on the offset of the center of the cursor in relation to the center of the model viewer. While the keyframe animation is on the.controls-prompt
the animation is paused and plays once the element isvisible
.Examples
Animated models
Robot Model
Animated models can also be rotated by the interaction prompt.
Auto rotate
If
auto-rotate
is enabled, interaction prompt will display if the user has not interacted with the model yet, after the model has been interacted with the prompt will go away andauto-rotate
will work as normal.Auto Rotate Interaction