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

Workaround for LaunchSites != StockLaunchSites issue #306

Merged
merged 3 commits into from
Aug 21, 2018

Conversation

marr75
Copy link
Contributor

@marr75 marr75 commented Aug 16, 2018

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.

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.

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);
Copy link
Member

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.

Copy link
Contributor Author

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.

Sorry, something went wrong.

Copy link
Member

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.

Sorry, something went wrong.

@@ -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() ;
Copy link
Member

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

Copy link
Contributor Author

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 :)

marr75 and others added 2 commits August 20, 2018 15:18
GetType -> typeof
var -> explicit type declaration
I need to build and test this still
@marr75
Copy link
Contributor Author

marr75 commented Aug 21, 2018

Built and tested with updates 👍
This fix will benefit a lot of GPP players. Thanks for being cool and making it easy to contribute!

@StollD
Copy link
Member

StollD commented Aug 21, 2018

Thanks for your contribution!

@StollD StollD merged commit bc4d0c5 into Kopernicus:master Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants