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

Display the flight planner and the orbit analyser in the tracking station #2536

Merged
merged 5 commits into from
Apr 18, 2020

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Apr 18, 2020

These two seem to make the tracking station much more useful.

The implementation involves a bit of code restructuring where we use PredictedVessel systematically in many places that were using ActiveVessel. Overall this factors out a bit of logic and fixes a buglet where the orbit analyser in the tracking station would show a stump of a window.

There is a remaining know issue for a Kerbal in EVA: if a flight plan is constructed in the tracking station, that flight plan is lost when switching to the map view. That's because the Kerbal becomes unready for a couple of frames. It doesn't happen in the other direction. We call this "the invariants of KSP". The ultimate fix would be to keep some of the state of unmanaged vessels, but that's a long term effort.

Fix #2531.

@@ -1587,8 +1591,7 @@ class PartCentredForceHolder {
// The only timing that satisfies these constraints is BetterLateThanNever
// in LateUpdate.
string main_vessel_guid = PredictedVessel()?.id.ToString();
if (MapView.MapIsEnabled && main_vessel_guid != null &&
PluginRunning() && plugin_.HasVessel(main_vessel_guid)) {
if (MapView.MapIsEnabled && main_vessel_guid != null && PluginRunning()) {
Copy link
Member

Choose a reason for hiding this comment

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

PredictedVessel requires PlugginRunning, but we are only testing that after calling it; either it was, and remains, useless here, or this is a fatal failure.

Add PluginRunning to the conjunction in PredictedVessel.

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.

}

// A helper for implementing the RenderButton() method of the subclasses.
protected void RenderButton(string text) {
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem specific to vessel-related windows: the plotting frame selectors have a button too.

Should there be RenderButton in the base class, overriden here (not necessarily in this pull request, but maybe file a cleanup issue).

Copy link
Member Author

Choose a reason for hiding this comment

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

Look at the code of this function: it is dependent on the existence of a predicted vessel. That's why it belongs here. This was horribly broken in the OrbitAnalyser, as the decision to hide was made in the middle of RenderWindow, a sure recipe for layout/repaint inconsistencies. This proves that the logic is non-obvious and belongs to a common place.

Sorry, something went wrong.

@eggrobin eggrobin added the LGTM label Apr 18, 2020
@pleroy pleroy merged commit c640b09 into mockingbirdnest:master Apr 18, 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.

Make orbital analysis window available in tracking station
2 participants