-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add support for editing burn parameters using a text field #2127
Conversation
actual_final_time - | ||
plugin_.FlightPlanGetInitialTime(vessel_guid))); | ||
} | ||
UnityEngine.GUILayout.TextField(message, style); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is either empty or orange, we might as well make it unconditionally orange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
set { | ||
if (!value_.HasValue || value_ != value) { | ||
value_ = value; | ||
formatted_value_ = formatter_(value_.Value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not proud of it. (I am, a bit.)
ksp_plugin_adapter/burn_editor.cs
Outdated
|
||
private double previous_burn_final_time => previous_burn_?.final_time ?? | ||
plugin_.FlightPlanGetInitialTime( | ||
vessel_.id.ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a confusing name, since it may not be previous_burn_.final_time
.
Call it time_base
, this is what we call it in the UI anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
private float slider_position_ = 0.0f; | ||
private DateTime last_time_; | ||
private double? value_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document the semantics of the optionality, or make it non-optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments.
@@ -72,15 +149,22 @@ internal class DifferentialSlider : ScalingRenderer { | |||
|
|||
if (UnityEngine.GUILayout.Button("0", GUILayoutWidth(1))) { | |||
value_changed = true; | |||
value = 0; | |||
// Force a change of value so that any input is discarded. | |||
value = zero_value_ + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the optional can help here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
if (slider_position_ != 0.0) { | ||
value_changed = true; | ||
// Moving the slider doesn't cause a loss of focus so we terminate | ||
// input if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall correctly, the issue is not that it doesn't cause a loss of focus (it does terminate input if it is another slider), rather that we start changing the value before committing the text input (perhaps because of event order?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that clicking on the slider/button of the same differential slider doesn't cause a loss of focus at all, and clicking elsewhere does. I don't feel like experimenting to nail down the exact semantics of Unity, though.
new DifferentialSlider( | ||
label : "t initial", | ||
unit : null, | ||
log10_lower_rate : Log10TimeLowerRate, | ||
log10_upper_rate : Log10TimeUpperRate, | ||
zero_value : 0.001, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deserves a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ksp_plugin_adapter/burn_editor.cs
Outdated
@@ -155,7 +157,7 @@ class BurnEditor : ScalingRenderer { | |||
specific_impulse_in_seconds_g0 = specific_impulse_in_seconds_g0_, | |||
frame = reference_frame_selector_.FrameParameters(), | |||
initial_time = previous_coast_duration_.value + | |||
previous_burn_final_time, | |||
time_base, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fits on the preceding line.
Fix #2121.