Skip to content

Conversation

SofieBrink
Copy link
Collaborator

  • Updated ModuleCoPFollowTransform to make better use of part prefabs while still supporting B9PS OnLoad calls.

Resolves #2

- Updated ModuleCoPFollowTransform to make better use of part prefabs while still supporting B9PS OnLoad calls.
@SofieBrink SofieBrink added the enhancement New feature or request label Oct 9, 2024
@SofieBrink SofieBrink requested a review from JonnyOThan October 9, 2024 14:40
@SofieBrink SofieBrink self-assigned this Oct 9, 2024
@SofieBrink
Copy link
Collaborator Author

Hey @JonnyOThan,
Would love if you had a look at this before i merge it, i think this should satisfy your remarks about the prefabs but would love to know for sure.

if (followTransform == null || followTransform.name != transformName)
{
if (transformName != null) followTransform = part.FindModelTransform(transformName);
if (followTransform == null) Debug.LogError($"[{MODULENAME}] transformName was empty or does not exist.");
Copy link
Contributor

@JonnyOThan JonnyOThan Oct 9, 2024

Choose a reason for hiding this comment

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

would be a good idea to include the transform name that was searched for in the error message.

Oh, right, I can just do that.

if (followTransform == null)
{
// this may be important if someone is swapping out versions of this module with B9PS
part.CoPOffset = Vector3.zero;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This makes sense but I feel like we might not want to set it to zero but to whatever it was in the part config itself, assuming there's a good way to get this at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

This actually isn’t a KSPField so can’t be configured in the cfg. But maybe grabbing the value from the prefab is better

Copy link
Collaborator Author

@SofieBrink SofieBrink Oct 10, 2024

Choose a reason for hiding this comment

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

Huh i swear i’ve seen it configured in the partconfig before, but the prefab should be exactly that i guess.

edit: yeah there’s like a bajillion parts on github here that define a CoPOffset in the part cfg. I assume that works.

Copy link
Contributor

@JonnyOThan JonnyOThan Oct 10, 2024

Choose a reason for hiding this comment

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

I don't see anything in Assembly-CSharp that would set it from the cfg. Maybe it's in FirstPass? Maybe it's done by reflection somehow.

Oooh yeah it is reflection. Didn't realize parts did that. Any top-level key in the PART cfg will bind to a public field on the Part class even if it doesn't have [KSPField] or [Persistent]

Given stock code, this should always be 0 for everything except parts with ModuleProceduralFairing.  But if some mod poked a different value into the prefab, this should be more correct.  And if a mod poked a different module into the part instance, then we're kinda screwed anyway.
@JonnyOThan JonnyOThan self-requested a review October 10, 2024 03:37
@SofieBrink SofieBrink merged commit 63ad0b4 into main Oct 11, 2024
1 check passed
@SofieBrink SofieBrink deleted the Update_ModuleCoPFollowTransform branch October 13, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

ModuleCoPFollowTransform could use prefabs better
2 participants