Skip to content

Improved RP-1 tech progression#2955

Merged
Dunbaratu merged 1 commit intoKSP-KOS:developfrom
RCrockford:rp1-avionics-tech-progression
Jul 13, 2021
Merged

Improved RP-1 tech progression#2955
Dunbaratu merged 1 commit intoKSP-KOS:developfrom
RCrockford:rp1-avionics-tech-progression

Conversation

@RCrockford
Copy link
Contributor

Changes RP-1 kOS cores to have tech progression that follows the user selected avionics tech.

This requires changes in RP-1 in addition for correct behaviour, but should be safe to merge before that as it will do nothing if the changes are not present.

@Dunbaratu
Copy link
Member

I just tried loading this into my RP-1 save (without the associated RP-1 PR, so I could verity that it doesn't break things if this is in kOS but the RP-1 part of it isn't there) and immediately ran into this problem - I cannot get the numbers to appear. It looks like this (two screenshots included):
screenshot21
screenshot22

Copy link
Member

@Dunbaratu Dunbaratu left a comment

Choose a reason for hiding this comment

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

I think I may have found why when I tested this on an existing RP-1 installation it caused the problems.

{
try
{
FindRP1Modules();
Copy link
Member

Choose a reason for hiding this comment

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

If FindRP1Modules() threw an exception (see other comment above in this review), then the rest of OnStart() would get skipped, causing all kinds of stuff to be uninitialized.

SafeHouse.Logger.Log(string.Format(" techNodeField type={0}", techNodePropInfo.PropertyType.Name));
var techNodeProp = techNodePropInfo.GetValue(RP1AvionicsModule);
var diskSpaceField = techNodeProp.GetType().GetField("kosDiskSpace");
SafeHouse.Logger.Log(string.Format(" techNodeField kosDiskSpace={0}", diskSpaceField.GetValue(techNodeProp).ToString()));
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a null protect check on this use of diskSpaceField.GetValue() like you have elsewhere in this PR for these new fields. (So if using an older RP-1 that doesn't have your PR in it, I think this line would throw a nullref exception.)

Changes RP-1 kOS cores to have tech progression that follows the user selected avionics tech.

This requires changes in RP-1 in addition for correct behaviour, but should be safe to merge before that as it will do nothing if the changes are not present.
@RCrockford RCrockford force-pushed the rp1-avionics-tech-progression branch from ab8c5bd to f08af0a Compare June 6, 2021 14:08
@RCrockford
Copy link
Contributor Author

The exception was in some debug logging code that isn't really required ironically. I have removed it, so it should be more robust now.

@RCrockford
Copy link
Contributor Author

The required changes in RP-1 have now been released, so that should make testing this a little easier.

@Dunbaratu Dunbaratu merged commit bfe79c9 into KSP-KOS:develop Jul 13, 2021
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.

2 participants