Investigate a possible (bad) iteraction with Procedural Parts (RO) V2.3.0

[Lisias]

Fellow Kerbonaut Galland1998 reported an issue about Procedural Parts when TweakScale is installed (here and here):

If a procedural part is the initial root part added to a craft then the attach nodes are offset from the part. If you add another of the same procedural part than the attach nods are in the proper place. If you make the second procedural part the root part and delete the original procedural part the attach nodes stay where they are supposed to be.

In the second case if the initial root part placed in the VAB is not a procedural part all of the nodes are where they are supposed to be and if you add a procedural part then the added procedural part has its nodes exactly where they are supposed to be.

RO/RP-1 messes with the base part scales compared to stock KSP so I am not sure if that has something to do with it. It is just strange that it only seems to impact an initial root part from the procedural parts mod.

[Lisias]

First things first. Lets check the behaviour on KSP 1.8.1 (the last KSP version where Editor works fine) with the latest TweakScale, the latest KSP-Recall and the Procedural Parts V.2.3:

The root part of the Cloned subtree (in green) is a Procedural Tank cone shaped:

This stablished that TweakScale and Procedural Parts are working fine on KSP 1.8.1. So there're no misbehaviours on these two Add'Ons. KSP-Recall's AttachedOnEditor is not applied on KSP 1.8.1 as it's not needed.

[Lisias]

Interesting enough, on KSP 1.9.1 (with the latest KSP-Recall and TweakScale), and the thing worked the same!

So, nope. There's nothing wrong on TweakScale and Procedural Part, and now we have evidence that KSP-Recall is also working as intended.

[Lisias]

Remark: the second part after the Procedural Part one does not have variants, what would trigger a problem on KSP >= 1.9.0 without KSP-Recall.

As we can see, the stack attachment nodes are working fine on this use case, known to trigger problems on KSP without KSP-Recall.

[Lisias]

In time, the AttachedOnEditor is being applied on the Procedural Part when using under KSP >= 1.9.0, as expected.

Test Craft: Untitled Space Craft.craft.zip

[DRVeyl]

Shorter form description of the issue:

Start with an empty VAB. Place any ProcParts root part. Without Tweakscale/KSP-Recall: the attachment nodes are in the correct position. With Tweakscale/KSP-Recall: the attachment nodes are offset from where they should be.

Not sure what you're attempting to fix in general, but I believe ProcParts handles its attachment nodes appropriately entirely on its own.

[Lisias]

@DRVeyl

Not sure what you're attempting to fix in general, but I believe ProcParts handles its attachment nodes appropriately entirely on its own.

Ah!! So that's the trigger!! The Procedural Part must be root!

The problem is a nasty screwup on KSP Editor since 1.9, and it's described in detail in issues:

• https://github.com/net-lisias-ksp/KSP-Recall/issues/32
• https://github.com/net-lisias-ksp/KSP-Recall/issues/34
• https://github.com/net-lisias-ksp/KSP-Recall/issues/35

I only (and finally) managed to grasp the real cause on around this post.

In essence, something broke while developing KSP 1.9 and Squad choose to work around the broke thingy: after OnLoad and before the First Update or FixedUpdate, the prefab values for Attachments Nodes are write into the parts - completely screwing up any values changed by any PartModule or read from the craft file.

While digging on the problem (for 2 years already!), I realised that the Editor's bug happens when the Root part is not a variant and the following part *is not a variant neither**. In any other situation, the Nodes are handled OK. My guess is that whoever did that crappy move of shoving back prefab values didn't managed to identify the exact reason the attachments weren't being initialised correctly, and so just shoved prefab values on the damned thing and called a day.

In a way or another, you just brought evidence that this problem is to be handled on KSP-Recall, and not on TweakScale! Once I confirm the information, I will move this issue to KSP-Recall,

[Lisias]

Unsurprisingly, there's no problems on KSP 1.8.1.

test_pp.zip

[Lisias]

ON KSP 1.9.1 , where AttachedOnEditor (the KSP-Recall component that could be involved on this hypothetical problem) things worked fine too.

[EDIT]: On this test, and every single one below, the craft were saved, loaded back, launched and reverted to SPH)

test_pp.zip

[Lisias]

On KSP 1.12.3 the same tests didn't revealed any problem, so I did some more using parts with Variants (what I already know it will works - but black-box testing is black-box testing). No misbehavior detected.

test_pp.zip

[Lisias]

On a clean install using only the latest KSP-Recall, the latest TweakScale, the latest Procedural Parts and Part Info, no misbehaviour were detected by reproducing the steps mentioned by @DRVeyl .

@DRVeyl I will need a craft file where the problem happens as well your KSP.log. The problem affecting you is something else, and the answer is on the KSP.log and the craft file.

[Lisias]

This issue, unfortunately, motivated a bogus pull request undully declaring Recall as conflicting with Procedural Parts.

https://github.com/KSP-CKAN/NetKAN/pull/9076

[StonesmileGit]

Steps to reproduce; Fresh KSP 1.12.3 install. Install Procedural Parts and TweakScale using CKAN GamaData:

Place Procedural Decoupler. Result:

Other parts also have bad nodes, but since the decoupler is shorter, it's more visible

[StonesmileGit]

Looks like the nodes are initialized as if the part is 1m tall when TS/KSPRecall is installed, here is the "Procedural Ore Tank" that defaults to 1m length:

[Lisias]

Place Procedural Decoupler.

Weirdly enough, the fuel Tanks behave when placed as root.

Other parts also have bad nodes, but since the decoupler is shorter, it's more visible

I tried Fuel Tanks and RCS Tanks. No misbehaviour were detected on these ones.

In time, I could reproduce the problem using the stack separator downto KSP 1.9.1:

On 1.8.1 (unsurprisingly), it behave.

Looks like the nodes are initialized as if the part is 1m tall when TS/KSPRecall is installed, here is the "Procedural Ore Tank" that defaults to 1m length:

Nice catch. The problem is happening due how Procedural Parts initialises itself! Using the same Stack Separator that misbehave, I could have it "fixed" by changing it's shape and changing back!

And this explains why I didn't reproduced the problem - just didn't passed to my mind someone would apply a procedural part just to use it on it's default values! :P So I just edited the procedural parts parameters to play a bit with the code.

We have a race condition here. Now it's a matter to know if the race condition is on AttachedOnEditor or in the Procedural Parts itself. [edit: nope. it was not it, see below]

Working on it.

[Lisias]

Found it.

This is what happens:

• AttachedOnEditor loads up, realises it's internal data structures are empty, and so refresh them from the current Part's Data
• Later, when the Procedural Part loads up these values are updated with valid ones (overwriting the ones AttachedOnEditor used to initialise itself)
  • By this time, AttachedOnEditor now have older, invalid values on its internal data structures
• AttachedOnEditor starts up, realises it's being started on Editor and apply its internal data into the Part's data to assure data integrity (as KSP's Editor screws up these data on certain circumstances since KSP 1.9.0).
• Procedural Parts Starts up, but don't bother to refresh the Part's data with valid values, unless you had switched the shape of the Part - a situation that triggers AttachedOnEditort to refresh its internal data structures and then everything is fine.

Before trying to fix somehow this race condition [edit: not a race condition, see below], I need to determine if AttachedOnEditor would be needed at all on Procedural Parts. Since it works fine on KSP 1.9.1 without Recall, it looks to me that the way Procedural Parts works internally is imune to the KSP >= 1.9.x Editor blunders.

So the first step is to determine if I could remove AttachedFromEditor from Procedural Parts and things still works fine for TweakScale (and any other hypothetical add'on that would neeed this).

Working on it.

[Lisias]

On a side note, the Procedural Parts' config are not helping

All of them are using the same node definitions:

PART
{
        name = proceduralBattery
        <yada yada yada>
        // --- node definitions ---
        node_stack_top=0,0.5,0,0,1,0,1
        node_stack_bottom=0,-0.5,0,0,-1,0,1
        node_attach=0,0,0.5,0,0,-1,1

PART
{
        name = proceduralHeatshield
        <yada yada yada>
        // --- node definitions ---
        node_stack_top=0,0.5,0,0,1,0,1
        node_stack_bottom=0,-0.5,0,0,-1,0,1
        node_attach=0,0,0.5,0,0,-1,1

PART
{
        name = proceduralStackDecoupler
        <yada yada yada>
        // --- node definitions ---
        node_stack_top=0,0.5,0,0,1,0,1
        node_stack_bottom=0,-0.5,0,0,-1,0,1

Things would be way easier for 3rd Parties' PartModules that need these values on the OnLoad phase of the Part's Life Cycle if these data would be the correct ones.

I expect this to be a problem to any Part Module that trusts the Part's data integrity at OnLoad, as does AttachedOnEditor.

[Lisias]

On the bright side, merely removing AttachedFromEditor from parts with the module ProceduralPart solved this problem.

Ideally, however, the Parts from Procedural Parts should have the node definitions correctly defined to match the default values. This would had prevented this problem, as well will prevent problems with any add'on that needs the Part's node definition to be valid on OnLoad .

[Lisias]

Moving this to KSP-Recall.

[Lisias]

Removing "unrelated" as I transferred this from TweakScale to Recall, and now it's related.

Being not a bug on Recall, I didn't marked it as such.

[Lisias]

Implemented on commit https://github.com/net-lisias-ksp/KSP-Recall/commit/5d3056e78b85d2ba6fa462002ead237010d01bfe

[Lisias]

Tested. It works, so I'm closing this.

[Lisias]

Just detected this issue on Procedural Parts's issue tracker.

https://github.com/KSP-RO/ProceduralParts/issues/315

[Lisias]

I had fixed Procedural Parts itself, so the problem is not more even with older versions of KSP-Recall.

I'm working on a pull request to the upstream, but while Procedural Parts doesn't accepts the Pull Request, and I work some final details on the next Release of KSP-Recall, the following patch will fix the problem:

// This patch fixes the Attachment Nodes definitions for the
// Procedural parts, fixing the following issues:
//
// * https://github.com/KSP-RO/ProceduralParts/issues/315
// * https://github.com/net-lisias-ksp/KSP-Recall/issues/41

@PART[proceduralBattery]:AFTER[ProceduralParts]
{
    @node_stack_top =   0,  0.1875, 0, 0,  1, 0, 1
    @node_stack_bottom= 0, -0.1875, 0, 0, -1, 0, 1
}

@PART[proceduralHeatshield]:AFTER[ProceduralParts]
{
    @node_stack_top =   0,  0.1, 0, 0,  1, 0, 1
    @node_stack_bottom= 0, -0.1, 0, 0, -1, 0, 1
}

@PART[proceduralNoseCone]:AFTER[ProceduralParts]
{
    @node_stack_top =   0,  0.3125, 0, 0,  1, 0, 1
    @node_stack_bottom= 0, -0.3125, 0, 0, -1, 0, 1
}

@PART[proceduralStackDecoupler]:AFTER[ProceduralParts]
{
    @node_stack_top =   0,  0.1, 0, 0,  1, 0, 1
    @node_stack_bottom= 0, -0.1, 0, 0, -1, 0, 1
}

@PART[proceduralStructural]:AFTER[ProceduralParts]
{
    @node_stack_top =   0,  0.375, 0, 0,  1, 0, 1
    @node_stack_bottom= 0, -0.375, 0, 0, -1, 0, 1
}

@PART[proceduralTankLiquid]:AFTER[ProceduralParts]
{
    @node_stack_top =   0,  0.375, 0, 0,  1, 0, 1
    @node_stack_bottom= 0, -0.375, 0, 0, -1, 0, 1
}

@PART[proceduralTankRCS]:AFTER[ProceduralParts]
{
    @node_stack_top =   0,  0.25, 0, 0,  1, 0, 1
    @node_stack_bottom= 0, -0.25, 0, 0, -1, 0, 1
}

@PART[proceduralTankXenon]:AFTER[ProceduralParts]
{
    @node_stack_top =   0,  0.15, 0, 0,  1, 0, 1
    @node_stack_bottom= 0, -0.15, 0, 0, -1, 0, 1
}

@PART[proceduralTankSRB]:AFTER[ProceduralParts]
{
    @node_stack_top =   0,  1.25, 0, 0,  1, 0, 1
    @node_stack_bottom= 0, -1.25, 0, 0, -1, 0, 1
}

@PART[proceduralTankOre]:AFTER[ProceduralParts]
{
    @node_stack_top =   0,  0.5, 0, 0,  1, 0, 1
    @node_stack_bottom= 0, -0.5, 0, 0, -1, 0, 1
}

Evidences:

Whole save directory (now with a more or less functional rocket made with procedural parts on the default size) test_pp.zip

KSP.log: KSP.log.zip

MM's ConfigCache : ConfigCache.cfg.zip

MM's Patch log: MMPatch.log.zip

GameData Contents (that can be also derived from the KSP.log):

Test drive craft (on the savegame):

— EDIT —

This setup was validated on KSP 1.9.1 . 3 times, as the first two I made some mistakes while trying to convert quaternions to eulers by hand,. just to realize all the nodes orientations are the same : 0 1 0 or 0 -1 0 :P

[Lisias]

The same patchset above was tested on KSP 1.12.3.

save: test_pp.zip

KSP.log : KSP.log.zip

MM ConfigCache: ConfigCache.cfg.zip

MM Patch log: MMPatch.log.zip

— EDIT

Updated the zips with data collected after fixing the nodes' orientation on the patch above.

[Lisias]

All the nodes on the root part are on the right place. In both KSP 1.9.1 and KSP 1.12.3, the Gamedata had

• KSP-Recall 0.2.2.3
  • exactly this one: https://github.com/net-lisias-ksp/KSP-Recall/releases/tag/RELEASE%2F0.2.2.3
• Procedural Parts 2.3
  • exactly this one: https://github.com/KSP-RO/ProceduralParts/releases/tag/v2.3.0

Additional add'ons (unrelated to the problem, but listed for the sake of completude)

• KSPe
• My fork of Module Manager (for faster and easier debugging logs)
• CraftManager + KXAPI (to make easier fixing current Assemblies after installing Recall the first time)
• TweakScale (as it is extremely dependant from KSP-Recall working right)
• Part Info.

On KSP 1.12.3 Procedural Wings is also installed for further testings.

[Lisias]

Shoved unrelated back, as it was definitively a mishap on Procedural Parts. :)

[Lisias]

A pull request was applied to Procedural Parts.

https://github.com/KSP-RO/ProceduralParts/pull/316

[Lisias]

Reopening, as I was informed here about how to cope with changing Nodes.

https://github.com/KSP-RO/ProceduralParts/pull/316#issuecomment-1104549795

https://github.com/KSP-RO/ProceduralParts/blob/master/Source/ProceduralAbstractShape.cs#L457

TODO: Implement this on Recall.

[Lisias]

After careful considerations, I concluded that there's no need to listen to the OnPartNodeMoved as AttachmentOnEditor refreshes its internal data on PartModule.OnSave. And it doesn't acts outside the Editor, where listening to this Event would be paramount.

[Lisias]

It was detected that RP-0 also had the same problems as PP (i.e., doesn't initialise the part config with valid attachment nodes). So I had to extend the fix to every part that would use PP's modules.

(hopefully) this was accomplished on commit https://github.com/net-lisias-ksp/KSP-Recall/commit/aa810341e94ca9efd613f336917877794a351968 .

It works fine on PP itself, but honouring my agreement on https://github.com/KSP-CKAN/NetKAN/pull/9076 to be a free beta tester for PP and whatever depends from it, I need more time to test the thing on RP-0 (and could be a good idea to do so on some others, if existent).

[Lisias]

Besides being unrelated, I choose to apply a fix on KSP-Recall - as the fix wasn't accepted on the PP's repository. So it's now an enhancement too.

[Lisias]

Final testings passed with flying colours. Closing/Fixed it.