KSP Recall

KSP Recall

345k Downloads

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

Lisias opened this issue ยท 32 comments

commented

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.

commented

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

commented

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

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,

commented

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.

commented

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:

screenshot68

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

screenshot68

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.

commented

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

screenshot39

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

commented

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.

screenshot40

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

commented

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

Screen Shot 2022-03-29 at 23 26 49

Test Craft: Untitled Space Craft.craft.zip

commented

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:

screenshot44

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!

screenshot48

screenshot47

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.

commented

Unsurprisingly, there's no problems on KSP 1.8.1.

screenshot69

test_pp.zip

commented

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)

screenshot42

screenshot43

test_pp.zip

commented

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.

screenshot13

screenshot14

screenshot15

screenshot17

test_pp.zip

commented

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.

commented

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

KSP-CKAN/NetKAN#9076

commented

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

Place Procedural Decoupler. Result:
image

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

commented

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

commented

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.

commented

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.

commented

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 .

commented

Moving this to KSP-Recall.

commented

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.

commented

Implemented on commit 5d3056e

commented

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:

screenshot49

screenshot50

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

Screen Shot 2022-04-19 at 16 54 09

Test drive craft (on the savegame):

screenshot52

โ€” 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

commented

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

KSP-RO/ProceduralParts#315

commented

The same patchset above was tested on KSP 1.12.3.

screenshot13

screenshot14

screenshot15

screenshot16

screenshot17

screenshot18

screenshot19

screenshot20

screenshot21

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.

commented

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

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.

commented

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

commented

A pull request was applied to Procedural Parts.

KSP-RO/ProceduralParts#316

commented

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

KSP-RO/ProceduralParts#316 (comment)

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

TODO: Implement this on Recall.

commented

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.

commented

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 aa81034 .

It works fine on PP itself, but honouring my agreement on KSP-CKAN/NetKAN#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).

commented

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.

commented

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