-
Notifications
You must be signed in to change notification settings - Fork 114
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
Workaround for LaunchSites != StockLaunchSites issue #306
Conversation
see https://bugs.kerbalspaceprogram.com/issues/19510 for more details, syncs `PSystemSetup.Instance.StockLaunchSites` and `PSystemSetup.Instance.LaunchSites` so that when they are used interchangeably in stock code, there's no ArgumentOutOfRangeException.
@@ -276,6 +276,13 @@ void Start() | |||
Destroy(city.gameObject); | |||
} | |||
} | |||
var stockLaunchSitesField = PSystemSetup.Instance.GetType().GetField("stocklaunchsites", BindingFlags.NonPublic | BindingFlags.Instance); |
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.
Instead of using the name of the field, you should detect it based on it's position and fieldtype, since the names of private fields can be subject to obfuscation (at least in the past this happened on some builds). Something like GetFields(BindingFlags.NonPublic | BindingFlags.Instance).FirstOrDefault(f => f.FieldType == typeof(LaunchSite[]))
should work even if the name of the field should change.
Also, I would prefer if you could use typeof(PSystemSetup)
instead of PSystemSetup.Instance-GetType()
. Saves some calls and looks nicer IMO.
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.
typeof(PSystemSetup)
is certainly superior in this scenario, .GetType
is just used more often because of how reflection is traditionally employed. I'll correct this, thank you!
I understand your point with name of the field but I disagree with the conclusion. Assembly-Csharp.dll
already does get put through an obfuscator, btw (the cheap one that puts no-op switch statements all over the place), which doesn't mean much since they could switch some time or consider it a done deal. The backing field's name is substantially more durable than it being the only private LaunchSite[]
field in the class, though. .FirstOrDefault(f => f.FieldType == typeof(LaunchSite[]))
would silently introduce a regression (or worse) if the developers added another private field of the same type.
If Kopernicus weren't version locked, it might be necessary to add exception handling, though. As is, it will be easier to resolve if it throws an exception while preparing for a future version. All in all, if that field changes, a dev will have to look into it, probably by inspecting the source.
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.
Ok, then let's see what will happen.
However, could you please add a try { } catch { }
around the code? Just to prevent it from causing the whole mod to fail.
@@ -276,6 +276,13 @@ void Start() | |||
Destroy(city.gameObject); | |||
} | |||
} | |||
var stockLaunchSitesField = PSystemSetup.Instance.GetType().GetField("stocklaunchsites", BindingFlags.NonPublic | BindingFlags.Instance); | |||
LaunchSite[] stockLaunchSites = (LaunchSite[])stockLaunchSitesField.GetValue(PSystemSetup.Instance); | |||
var updateStockLaunchSites = stockLaunchSites.Where(site => !Templates.RemoveLaunchSites.Contains(site.name)).ToArray() ; |
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.
Tbh. I would prefer using the full type name instead of var
, but thats no a real issue
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.
Very different styles, I actually deeply regret the type declaration on line 280. However, this project's style is to explicitly declare the type so I will fix it on lines 279 and 281 :)
GetType -> typeof var -> explicit type declaration I need to build and test this still
Built and tested with updates 👍 |
Thanks for your contribution! |
See https://bugs.kerbalspaceprogram.com/issues/19510 for more details, syncs
PSystemSetup.Instance.StockLaunchSites
andPSystemSetup.Instance.LaunchSites
so that when they are used interchangeably in stock code, there's no ArgumentOutOfRangeException.Tested against Galileo's Planet Pack with expansion installed. Stepped through the code to ensure the launch site lists were in sync and no longer encountered the bug.
Couldn't really avoid reflection since the collection that needs to be fixed is an array property backed by a private field. Happens once so performance is not a big deal but will be sensitive to name changes. Could have assigned
PSystemSetup.Instance.LaunchSites.ToArray()
to the private field to keep them in sync but I think this is more future proof if Squad makes changes to how these collections are handled in the future.