-
Notifications
You must be signed in to change notification settings - Fork 871
Auto poster generation #2711
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
Auto poster generation #2711
Conversation
++this.scaleStep; | ||
} else if ( | ||
this.avgFrameDuration < LOW_FRAME_DURATION_MS && this.scaleStep > 0) { | ||
--this.scaleStep; | ||
} | ||
this.scaleStep = Math.min(this.scaleStep, this.lastStep); |
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 ensures minScale will be respected immediately on the next render.
@@ -469,6 +469,8 @@ export const LoadingMixin = <T extends Constructor<ModelViewerElementBase>>( | |||
this.dispatchEvent(new CustomEvent('poster-dismissed')); | |||
}); | |||
}, {once: true}); | |||
} else { | |||
this[$transitioned] = true; |
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.
Found a small bug where when things changed too quickly, the transitioned event would not fire and then the state would get messed up. This ensures the state is correct for the poster to dismiss.
@@ -180,6 +182,10 @@ export class OpenMobileView extends ConnectedLitElement { | |||
|
|||
await post(JSON.stringify(packet), getSessionUrl(this.pipeId, session.id)); | |||
|
|||
await post( | |||
posterBlob, | |||
posterToSession(this.pipeId, session.id, updatedContent.posterId)); |
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 post is what allows the poster to function on mobile (it was set to an objectURL before, but that can't be accessed from a different device; it must be explicitly sent over the network).
} | ||
|
||
const SET_MIMETYPE = 'SET_MIMETYPE'; | ||
export function dispatchMimeType(type: ImageType) { |
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 put this in because I thought I'd allow choosing PNG, JPEG, WEBP, but then I realized WEBP is definitely the best, so I removed the UX. I left the logic in case it's useful down the road.
// Set to beginning of animation | ||
const oldTime = modelViewer.currentTime; | ||
modelViewer.autoplay = false; | ||
modelViewer.currentTime = 0; |
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 make sense to have reset or init function for this part?
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.
Maybe, but since the reset is pretty specific to the poster generation I feel like it's simpler to contain the logic here. If it comes in hand anywhere else, I'll definitely put it in a function.
I removed the semi-functional poster UX in the editor and now the poster is auto-generated on any save and whenever it is deployed to mobile. In fact posters were not working on mobile at all before, but now they do, and they improve the experience since they demonstrate the use while the slow piping server downloads. I'm also generating only WebP images, since they are supported universally now (since IE is dead) and they have compact lossy compression with an alpha channel.