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

Added drawing style configuration #2816

Merged
merged 4 commits into from
Dec 11, 2020

Conversation

RCrockford
Copy link
Contributor

Adds user configurable drawing styles for trajectories via an MM patch.

I think this addresses #869.

@pleroy
Copy link
Member

pleroy commented Dec 11, 2020

[Automated message from GitHub Pull Request Builder] Answer "ok to test" to trigger testing of this PR.

ksp_plugin_adapter/ksp_plugin_adapter.cs Outdated Show resolved Hide resolved
ksp_plugin_adapter/ksp_plugin_adapter.cs Outdated Show resolved Hide resolved
ksp_plugin_adapter/ksp_plugin_adapter.cs Outdated Show resolved Hide resolved
@RCrockford RCrockford force-pushed the draw-style-configuration branch from 4c1d15c to 5650d7e Compare December 11, 2020 12:38
ksp_plugin_adapter/ksp_plugin_adapter.cs Outdated Show resolved Hide resolved
return;
}
ConfigNode history_parameters = draw_styles.GetAtMostOneNode("history");
if (history_parameters != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Remove these if statements, use history_parameters?.GetDrawMeow instead (technically this might mean twice the null checks but I’d rather reduce boilerplate than optimize null checks in something that runs in OnAwake.

Copy link
Member

Choose a reason for hiding this comment

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

Wait no, ?. worked with the out parameters, but not now that we return values.

Copy link
Member

@eggrobin eggrobin Dec 11, 2020

Choose a reason for hiding this comment

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

Upon reflection, I think I preferred the single function that loaded the entire style node.

Please merge

UnityEngine.Color GetDrawColour(this ConfigNode node)

and

GLLines.Style GetDrawStyle(this ConfigNode node)

into

void GetDrawStyle(this ConfigNode node, out UnityEngine.Color colour, out GLLines.Style style)

at which point the callsites can become

history_parameters?.GetDrawStyle(out history_colour, out history_style)

etc.

@eggrobin eggrobin added the LGTM label Dec 11, 2020
@eggrobin eggrobin merged commit a25af81 into mockingbirdnest:master Dec 11, 2020
@RCrockford RCrockford deleted the draw-style-configuration branch December 11, 2020 15:30
@pleroy
Copy link
Member

pleroy commented Dec 12, 2020

I can haz some documentation in the Wiki (there is a section about configuration files)? We cannot expect users to read the code to figure out how to play with the colours.

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.

None yet

3 participants