playback speed control #445
Conversation
| export const DEFAULT_PLAYBACK_SPEED_INDEX = 3; | ||
| export const PLAYBACK_SPEEDS: number[] = [5, 10, 15, 20, 30, 45, 60]; |
There was a problem hiding this comment.
Somewhat arbitrary set of options any number can be provided to the prop, but the playback speed will be throttled by the animation frame rate to 60 at the highest.
Could use a slider for scrubbing speed, or whatever UX wants in production.
| interface PlaybackControlsProps { | ||
| simulariumController: any; | ||
| totalSteps: number; | ||
| timeStep: number; | ||
| firstFrameTime: number; | ||
| currentFrame: number; | ||
| setSpeed: (speed: number) => void; | ||
| currentSpeed: number; | ||
| } | ||
|
|
||
| const PlaybackControls: React.FC<PlaybackControlsProps> = ({ | ||
| simulariumController, | ||
| totalSteps, | ||
| timeStep, | ||
| firstFrameTime, | ||
| currentFrame, | ||
| setSpeed, | ||
| currentSpeed, | ||
| }) => { |
There was a problem hiding this comment.
This component includes the new speed controls, and the other existing playback controls, small refactor as part of cleaning up test-bed.
src/viewport/index.tsx
Outdated
| private currentSimulationTime: number; | ||
| private clockTimePerRender: number; | ||
| private clockTimeSinceRender: number; | ||
| private totalClockTimeElapsed: number; | ||
| private clockTimeSinceFrameAdvance: number; | ||
| private clockTimePerFrameAdvance: number; |
There was a problem hiding this comment.
These will make more sense below, but as part of refactoring the animate method I pulled out variables that were scoped within that function to make them available to the whole class, and tried to name them more explicitly.
src/viewport/index.tsx
Outdated
| public animate(): void { | ||
| const { simulariumController } = this.props; | ||
| this.updateAnimationTime(); | ||
| if (this.clockTimeSinceRender <= this.clockTimePerRender) { | ||
| this.animationRequestID = requestAnimationFrame(this.animate); | ||
| return; | ||
| } | ||
| if (simulariumController.isChangingFile) { | ||
| this.handleFileChange(this.totalClockTimeElapsed); | ||
| return; | ||
| } | ||
| const currentFrame = simulariumController.visData.currentFrameData; | ||
| this.renderCurrentFrame(currentFrame); | ||
|
|
||
| if (this.shouldFrameAdvance()) { | ||
| this.advanceFrame(); | ||
| } | ||
| this.stats.begin(); | ||
| this.visGeometry.render(this.totalClockTimeElapsed); | ||
| if (this.recorder?.isRecording) { | ||
| this.recorder.onFrame(); | ||
| } | ||
| this.stats.end(); | ||
| this.lastRenderTime = Date.now(); |
There was a problem hiding this comment.
Initially I was piling new code into this already somewhat hard to read function, but as I was chasing bugs I figured it might be time to factor it out on behalf of readability. It does a lot and I think having helper methods makes this easier to understand what is going on.
interim17
requested review from
meganrm and
ShrimpCryptid
and removed request for
a team
src/viewport/index.tsx
Outdated
| private currentSimulationTime: number; | ||
| private clockTimePerRender: number; | ||
| private clockTimeSinceRender: number; | ||
| private totalClockTimeElapsed: number; | ||
| private clockTimeSinceFrameAdvance: number; | ||
| private clockTimePerFrameAdvance: number; |
There was a problem hiding this comment.
Maybe needs a comment saying what units these are in, or renaming (ex clockTimePerRenderMs)
src/viewport/index.tsx
Outdated
| this.clockTimeSinceRender = now - this.lastRenderTime; | ||
| this.totalClockTimeElapsed = now - this.startTime; | ||
| this.clockTimeSinceFrameAdvance += this.clockTimeSinceRender; |
There was a problem hiding this comment.
I find it a little strange that this.clockTimeSinceRender and this.totalClockTimeElapsed are saved as private variables since they can be derived on the fly and are only used in one or two places. You could have a getClockTimeSinceLastRender() method that does the same thing, or have this method return the calculated values.
this.clockTimeSinceFrameAdvance also seems like it should just be a calculated value since it's only used in one place, and instead you could save the timestamp of the last frame advance as your private variable instead of having to add up the time deltas. (It would also be slightly more accurate since adding up deltas can be inconsistent.)
There was a problem hiding this comment.
I really appreciate this feedback.
Did a second pass after naming methods and they also seemed strange and one off...
Did a small refactor to use local variables inside animate (as before), but also utilize the timestamp from requestAnimationFrame to keep things in sync.
src/viewport/index.tsx
Outdated
| @@ -402,6 +420,9 @@ class Viewport extends React.Component< | |||
| this.visGeometry.destroyGui(); | |||
| } | |||
| } | |||
| if (playbackSpeed !== prevProps.playbackSpeed) { | |||
| this.clockTimePerFrameAdvance = 1000 / playbackSpeed; | |||
There was a problem hiding this comment.
Do you need to store clockTImePerFrameAdvance or can we just calculate it off of props.playbackSpeed each time?
Time Estimate or Size
small
Problem
Users want to control playback speed
Advances #424
Advances #446
Continuation of prefetching work and pointing at that feature branch pending review.
This feature still needs UX input and was not prioritized, but basically was building it was basically part of debugging the prefetching.
Website implementation pending UX input.
Solution
Added a viewer prop to let the front end set playback speed in frames per second.
Added
shouldFrameAdvanceandadvanceFramemethods to viewport to control playback speedRefactored the
animateloop by pulling some variables out, and splitting it into smaller functions that make explicit what each part of the loop is doing.Added front end controls in test-bed to preview feature.
Added
PlaybackControlscomponent to test-bed to hold previously existing controls, and new speed control.Change summary:
Steps to Verify: