prefetching #442
Open
prefetching #442
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…acilitate prefetching
…r instead of time, callbacks for streaming changes
…pusServicesClient abstraction
…mularium-viewer into feature/isimulator
…mularium-viewer into feature/prefetching
…into feature/prefetching
interim17
commented
Jan 30, 2025
| @@ -70,7 +72,7 @@ interface ViewerState { | |||
| agentColors: number[] | string[]; | |||
| showPaths: boolean; | |||
| timeStep: number; | |||
| totalDuration: number; | |||
| totalSteps: number; | |||
There was a problem hiding this comment.
First example of a move away from time values towards frame values, see comments in write up!
interim17
commented
Jan 30, 2025
Comment on lines
+823
to
+832
| min={0} | ||
| step={1} | ||
| value={this.state.currentFrame} | ||
| max={this.state.totalSteps} | ||
| onChange={this.handleScrubFrame} | ||
| /> | ||
| <label htmlFor="slider"> | ||
| {this.state.currentTime} / {this.state.totalDuration} | ||
| {this.state.currentFrame * this.state.timeStep + | ||
| this.state.firstFrameTime} | ||
| / {this.state.totalSteps * this.state.timeStep} |
There was a problem hiding this comment.
The slider just uses the frame numbers as it's values and derives times for display values.
interim17
requested review from
ShrimpCryptid and
tyler-foster
and removed request for
a team
src/controller/index.ts
Outdated
| @@ -343,6 +412,7 @@ export default class SimulariumController { | |||
| // calls simulator.abort() | |||
| this.stop(); | |||
|
|
|||
| // this.abortRemoteSimulation(); | |||
ShrimpCryptid
approved these changes
Feb 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Time Estimate or Size
large
fix for lint warnings in another open PR
Problem
Prefetching frames is continuation of caching upgrades, and necessary for front end features like buffering indicators and playback speed settings.
Solution
Conceptually, we are establishing a distinction between the "playback head" and the "streaming head", which will get fleshed out with more complex caching behavior, some forthcoming Octopus adjustments, and the ability to move in and out of live mode.
I'm keeping controller method names and functionality intact to avoid breaking changes. That said, I have some opinions about some practices I'd like to move away from, primarily that I'd like to request frames, and use frame number, whenever possible, in preference over using simulation time. If the PR doesn't make it clear why, or if anyone disagrees, I am happy to discuss!
CacheAndStreamingLogs
This component may not need to be a permanent part of the test-bed, could be deleted/retired once some critical caching work is done, along with its callback. But for now getting some detailed readouts from the cache are essential for dev quality of life. Production front ends don't need to subscribe to the cache logs if they are unneeded, and the test bed readout is hidden until revealed. Better design/appearance TBD. Also changes to the recording component are just to save some visual space.
totalDuration / totalSteps and the general frame vs time issue
Time values like
totalDurationin the test bed are derived fromtotalStepsand thetimeStep. When we use a time value to request frames from the backend, we first have to derive it from frame number, and then Octopus has to convert it back into a frame number. Using this type of derived value for elements like the slider adds floats where we could be dealing with ints and potential front end back end sync issues. I think we should move away from callinggotoTimeand towards using the newmovePlaybackFrame. The functionality ofgotoTimehas been retained.playing/streaming states and methods
Playback tracked via
isPlayinginstead ofisPausedand both states held at controller level.Some functions related to playback have been retained with the same naming (
resume/pause) although I would like to make them more explicit in the future like:pausePlaybackor evenpausePreComputedSimandpauseLiveSimas we distinguish playback modes.onStreamingChange
To subscribe the front end to changes in when we are streaming or not
onCacheUpdate
Subscribe the front end to logging from the cache, need is dev only for now, so
currentStreamingHeadis distinguished in visData as the piece necessary for productiongetFrameAtTime
gotoTimeis still a thing, as issimulator.requestFrameByTimebut under the hoodgotoTimeis routed through this function, and then tomovePlaybackFrame. When callinggotoTimethe simulator does not actually request a frame by time.remoteStreamingHeadPotentiallyOutOfSync
The async nature of our websocket stream leaves the small possibility that the viewer and the backend get out of sync in terms of what the "current" frame is in cases when when a frame has been rejected due to cache limit being reached.
This code to handle this behavior without major issues, but a long term fix to this is coming, as we will update Octopus messaging to include the ability to reset the backend's "current frame" without requesting a frame (currently the only way to do that).
Steps to Verify:
Show cache and streaming infobutton. The current default will cache about 15 frames of the COVID trajectory which is my usually test trajectory. Try playing, pausing, and advancing/retreating frames.