Skip to content
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

Timeline #157

Merged
merged 24 commits into from
Dec 29, 2024
Merged

Timeline #157

merged 24 commits into from
Dec 29, 2024

Conversation

twof
Copy link
Contributor

@twof twof commented Dec 22, 2024

Summary

The intent is to help users debug by allowing them to move both forward and backward through simulations. Users can currently only move forward.

New

  • Users can now reverse through the simulation with the "P" hotkey
  • Users can now scrub forward and backward through the simulation
Screen.Recording.2024-12-21.at.10.13.38.PM.mov

Memory impact

This change causes the system to cache all snapshots, where previously snapshots were loaded as needed and unloaded as soon as they were rendered. Running the Planetary Defense scenario with the default solution, 178MB are used when snapshots are cached, and 38MB is used without caching as in status quo.

Comment on lines +289 to 293
if self.steps_forward > 0 {
self.steps_forward -= 1;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't completely sure why steps forward was decremented down here rather than up above with the rest of the step forward logic. Should I decrement steps_backward down here as well?

I'm also curious under what circumstances steps_forward is greater than 1?

let mut delta = (self.physics_time - t).min(Duration::from_millis(16));

// Find the difference between current time and snapshot time, with a max value of 16
// TODO: Why 16? The tick length is 16.66 miliseconds. Maybe that's it?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure about this, but I want to document it while I'm here.

Comment on lines +502 to 509
// TODO: Unsure what this is for
// TODO: More magic numbers
if delta > Duration::from_millis(3) {
delta -= Duration::from_millis(1);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also want to document this

Comment on lines +676 to +684
pub fn snapshot_count(&self) -> usize {
self.snapshots.len()
}

pub fn snapshot_index(&self) -> usize {
(self.snapshot.as_ref().map_or(0.0, |s| s.time) / PHYSICS_TICK_LENGTH).round() as usize
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are exposed for the UI

@@ -60,9 +60,10 @@ impl yew_agent::Worker for SimAgent {
self.link.respond(who, Response::Snapshot { snapshot });
}
Request::Snapshot { ticks, nonce } => {
if self.errored {
if self.errored || self.sim().status() != Status::Running {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This keeps responding with snapshots after the end of the simulation if we don't guard against it.

pending_snapshots: VecDeque<Snapshot>,
snapshots: Vec<Snapshot>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer need this to be a Deque. I was able to shave off 19% of used memory in a benchmark with this change.

Copy link
Owner

@rlane rlane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you squash together the commits and mark the PR as non-draft? Then I can merge.

@twof twof marked this pull request as ready for review December 29, 2024 23:30
@twof
Copy link
Contributor Author

twof commented Dec 29, 2024

@rlane Alright you're good to go. You can squash the commits through github's UI

Screenshot 2024-12-29 at 3 40 45 PM

@rlane rlane merged commit e54140d into rlane:master Dec 29, 2024
2 checks passed
@rlane
Copy link
Owner

rlane commented Dec 29, 2024

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants