-
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
Use C# 8 and the nullable reference type specification in the adapter #2312
Conversation
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.
Fix the generator to add ?
as appropriate to interface.generated.cs
.
ksp_plugin_adapter/flight_planner.cs
Outdated
@@ -77,7 +77,7 @@ class FlightPlanner : SupervisedWindowRenderer { | |||
} else if (UnityEngine.GUILayout.Button("Create flight plan")) { | |||
plugin.FlightPlanCreate(vessel_guid, | |||
plugin.CurrentTime() + 1000, | |||
vessel_.GetTotalMass()); | |||
vessel_!.GetTotalMass()); |
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.
Remove, see above.
@@ -128,7 +128,7 @@ class FlightPlanner : SupervisedWindowRenderer { | |||
} | |||
|
|||
if (burn_editors_ != null) { | |||
string vessel_guid = vessel_?.id.ToString(); | |||
string vessel_guid = 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.
Can we have a comment explaining why vessel_
cannot be null here?
I find this function very confusing in general, perhaps it should be split in a future pull request.
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 first order logic annotations.
@@ -21,7 +21,7 @@ internal class DisposableIteratorMarshaller : ICustomMarshaler { | |||
if (managed_object == null) { | |||
return IntPtr.Zero; | |||
} | |||
var disposable_iterator = managed_object as DisposableIterator; | |||
var disposable_iterator = (managed_object as DisposableIterator)!; |
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.
(DisposableIterator)managed_object
, which checks.
@@ -50,7 +50,7 @@ internal class DisposablePlanetariumMarshaller : ICustomMarshaler { | |||
if (managed_object == null) { | |||
return IntPtr.Zero; | |||
} | |||
var disposable_planetarium = managed_object as DisposablePlanetarium; | |||
var disposable_planetarium = (managed_object as DisposablePlanetarium)!; |
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.
(DisposablePlanetarium)managed_object
.
private KSP.UI.Screens.DebugToolbar.DebugScreen debug_screen_; | ||
private KSP.UI.Screens.DebugToolbar.Screens.Cheats.HackGravity hack_gravity_; | ||
private KSP.UI.Screens.DebugToolbar.DebugScreen? debug_screen_; | ||
private KSP.UI.Screens.DebugToolbar.Screens.Cheats.HackGravity? hack_gravity_; | ||
private KSP.UI.Screens.DebugToolbar.Screens.Cheats.HackGravity hack_gravity { |
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.
The property is also a HackGravity?
, because there only is a HackGravity
once someone opens the cheats menu; this is why we get the debug_screen_
and search within it.
See also line 420:
Principia/ksp_plugin_adapter/ksp_plugin_adapter.cs
Lines 420 to 422 in a3b2358
if (vessel.loaded && hack_gravity?.toggle.isOn == true) { | |
reasons.Add("vessel is loaded and gravity is being hacked"); | |
} |
@@ -457,7 +457,7 @@ private enum UnityLayers { | |||
} | |||
|
|||
// Returns false and nulls |texture| if the file does not exist. | |||
private bool LoadTextureIfExists(out UnityEngine.Texture texture, | |||
private bool LoadTextureIfExists(out UnityEngine.Texture? texture, |
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.
Apparently there is [NotNullWhen(true)]
for that: https://devblogs.microsoft.com/dotnet/try-out-nullable-reference-types/#conditional-postconditions-maybenullwhenbool-and-notnullwhenbool.
@@ -789,7 +791,7 @@ private enum UnityLayers { | |||
var normal = | |||
(Vector3d)plugin_.VesselBinormal(active_vessel.id.ToString()); | |||
|
|||
SetNavballVector(navball_.progradeVector, prograde); | |||
SetNavballVector(navball_!.progradeVector, prograde); |
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.
Comment why this is not null (I think because RenderNavball
set it above; we might want to make it a property like the Krakensbane etc. in the future for clarity).
@@ -1186,7 +1188,7 @@ private enum UnityLayers { | |||
new Origin{reference_part_is_at_origin = | |||
FloatingOrigin.fetch.continuous, | |||
reference_part_is_unmoving = | |||
krakensbane_.FrameVel != Vector3d.zero, | |||
krakensbane!.FrameVel != Vector3d.zero, |
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.
Same.
@@ -1603,7 +1606,7 @@ private enum UnityLayers { | |||
// Texture the ball. | |||
navball_changed_ = false; | |||
if (plotting_frame_selector_.target_override) { | |||
set_navball_texture(target_navball_texture_); | |||
set_navball_texture(target_navball_texture_!); |
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.
Comment that the textures are set in OnAwake
(we could write lazy-load properties for them so that things are clearer).
ConfigNode ephemeris_parameters = | ||
numerics_blueprint.GetAtMostOneNode("ephemeris"); | ||
ConfigNode? ephemeris_parameters = | ||
numerics_blueprint!.GetAtMostOneNode("ephemeris"); |
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.
Is this !
necessary with the null check above?
@@ -2121,7 +2124,7 @@ private enum UnityLayers { | |||
node => node.GetUniqueValue("name")); | |||
BodyProcessor insert_body = body => { | |||
Log.Info("Inserting " + body.name + "..."); | |||
if (!name_to_gravity_model.TryGetValue( | |||
if (!name_to_gravity_model!.TryGetValue( |
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.
Change lines 2111 .. 2113 from
if (name_to_gravity_model == null) {
Log.Fatal("Cartesian config without gravity models");
}
to
if (name_to_gravity_model == null) {
throw Log.Fatal("Cartesian config without gravity models");
}
so that control flow analysis understands that name_to_gravity_model
is not null here.
if (name_to_gravity_model?.TryGetValue( | ||
body.name, | ||
out body_gravity_model) == true) { | ||
Log.Info("using custom gravity model"); | ||
} | ||
Orbit orbit = unmodified_orbits_.GetValueOrNull(body); | ||
Orbit? orbit = unmodified_orbits_!.GetValueOrNull(body); |
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.
Comment that unmodified_orbits_
has been set in OnAwake
(we should probably create the dictionary at construction so that unmodified_orbits_
is not null, and just fill it in OnAwake
, but we can clean that up later).
@@ -114,7 +114,7 @@ internal abstract class BaseWindowRenderer : ScalingRenderer, IConfigNode { | |||
var old_skin = UnityEngine.GUI.skin; | |||
if (skin_ == null) { | |||
UnityEngine.GUI.skin = null; | |||
skin_ = MakeSkin(UnityEngine.GUI.skin); | |||
skin_ = MakeSkin(UnityEngine.GUI.skin!); |
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 case where we would really need to annotate the engine declarations:
the property UnityEngine.GUI.skin
is UnityEngine.GUISkin
, not UnityEngine.GUISkin?
, but it is [AllowNull]
(set
accepts null, get
never returns null).
No description provided.