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

Skip the points of the parts's psychohistory that are before the vessel's psychohistory fork #2516

Merged
merged 7 commits into from
Apr 1, 2020

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Mar 31, 2020

Also write the Principia version to the journal.

Fix #2490.
Fix #2507.

@@ -575,6 +591,7 @@ void Vessel::SwapPrognostication(
}

void Vessel::AppendToVesselTrajectory(
std::optional<Instant> const& fork_time,
Copy link
Member

Choose a reason for hiding this comment

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

We pass a DiscreteTrajectory, so we can compute this ourselves instead of making the caller do it.

Remove the parameter, and let

bool const after_fork_time =
    !at_end_of_part_trajectory && (trajectory.is_root() || first_time > trajectory().Fork()->time);

(or maybe hoist it as an optional out of the loop, but given that the ancestry is always shallow here this is probably not useful).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. is_root is cheap, Fork is not.

@@ -34,6 +36,19 @@ void Recorder::WriteAtDestruction(serialization::Method const& method) {
void Recorder::Activate(base::not_null<Recorder*> const recorder) {
CHECK(active_recorder_ == nullptr);
active_recorder_ = recorder;

// When the recorder gets activated, pretend that we got a GetVersion call.
Copy link
Member

Choose a reason for hiding this comment

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

Why pretend? Couldn’t we just call principia__GetVersion here instead of reimplementing it?

We might have to move it to its own interface_something file for cleaner includes, but splitting the main interface file tends to be something we want to do anyway…

Copy link
Member Author

Choose a reason for hiding this comment

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

I started along that path and it's not nice. The API of the interface is not exactly designed for being called from C++. In this case the parameters are char const** const, which would require awkward storage management. It's much simpler to just set two fields of a proto.

Sorry, something went wrong.

@eggrobin eggrobin added the LGTM label Apr 1, 2020
@pleroy pleroy merged commit 4098b8a into mockingbirdnest:master Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undocking causes gamecrash Appending before fork time
2 participants