Set field values before moving nodes

[HebaruSan]

Background

I'm working on the rewrite of SmartTank (see HebaruSan/SmartTank#3), and I've got it largely working (with one exception to be described below). To review, SmartTank is a mod that builds upon ProceduralParts by adding the ability to automatically resize tanks to achieve particular TWR targets, using KSP's delta V calculation engine (fomerly using KER's). It can also automatically adjust the tank diameter to fit attached parts.

The approach I'm taking in the rewrite is to call ProceduralPart.SeekVolume to update the tank lengths, and OnShapeDimensionChanged of whichever shape class when I change the diameters.

Problem

Placing procedural parts in symmetry while the above-described SmartTank rewrite is active results in incorrect placement of some of the attached parts:

In this image, I first built a simple middle stage, then placed a pair of radial decouplers on it, then procedural tanks, then engines under the side tanks. The image is captured immediately after the engines are added. The tank on the right is translated about twice as far as it's supposed to be horizontally in response to the diameter changing from 1.25m to 2.5m, and the engine on the right is translated downwards twice as far as it's supposed to be in response to the length changing to match the target TWR.

Cause

When the tanks are resized, the attach nodes have to be moved, and some of them are moving twice as far as they're supposed to. And it seems to only affect parts in symmetry. This suggests that the intended offset is being applied twice somehow for some parts in symmetry.

I tried a workaround in SmartTank code to update the size of only one of the tanks in the symmetry group to see whether the changes would be applied to it by some part of ProceduralParts, but this only made the problem worse; the tanks ended up different sizes and the attach nodes were still translated way off to the wrong place. After similarly failing in a few other workaround attempts, I started to dig into the PP code.

I think the duplicate call is in ProceduralAbstractShape.OnShapeDimensionChanged, which first calls TranslateAttachmentsAndNodes for the current part and then for all the other parts in its symmetry group. If you call SeekVolume for each part in a symmetry group, TranslateAttachmentsAndNodes is called multiple times for each of them. TranslateAttachmentsAndNodes works with relative position offsets rather than absolute, so the "extra" calls move the nodes further out by the same amount each time.

Changes

Now ProceduralAbstractShape.OnShapeDimensionChanged no longer calls TranslateAttachmentsAndNodes for other parts in symmetry with the current part; it will only be called when we are actually setting the size of the tank. In my testing this one change completely solves the above issue and makes SmartTank work flawlessly.

If this change breaks something else in ProceduralParts, please let me know, and I'll have another look at it with that broken thing in mind. If it looks OK, then I'd kindly request a new release be made before too long, so I can release the SmartTank rewrite to work alongside it.

Now ProceduralAbstractShape.SeekVolume and ProceduralShapePill.SeekVolume propagate field changes before the nodes are moved. This ensures that the nodes are moved to the correct place for length changes. The fix for diameter changes will be done in SmartTank by only calling OnShapeDimensionChanged once per symmetry group.

Tagging @DRVeyl in hopes of starting a conversation about this proposal.

[DRVeyl]

I recall that I spent a while during the original 2.0 rewrite to get the attachment node manipulation solid, and address issues exactly like what you refer to when operating on parts in symmetry. Not calling TranslateAttachmentsAndNodes in OnShapeDimensionChanged should cause symmetry parts to not properly resize and push their attachments when manipulating one of the symmetry components.

SeekVolume definitely was not tested with the idea of calling it on multiple parts in symmetry. If you only call it on one part, and rely on OnDimensionShapeChanged handler to properly manipulate all the symmetry parts, do you get correct behaviour? SeekVolume itself is really only a shorthand tool. https://github.com/KSP-RO/ProceduralParts/blob/master/Source/ProceduralAbstractShape.cs#L229-L243

Edit: There is some underlying issue if iteratively calling SeekVolume on multiple parts in symmetry produces the result above. I would expect that the first call changes everything's size, and the subsequent call calculates a new set of dimensions that should be 0 delta. Everything gets pushed again, but not an original distance. So still something strange in the flow there.

[DRVeyl]

Your approach should be fine, btw, with calling OnShapeDimensionChanged to adjust a diameter, and calling SeekVolume to let it sort out the length. It doesn't hit lengths perfectly (because of field precision), but does let you pick whether to overshoot, undershoot, or round to nearest. You could of course also call OnShapeDimensionChanged again to adjust length if you wanted to manipulate directly.

[HebaruSan]

SeekVolume definitely was not tested with the idea of calling it on multiple parts in symmetry. If you only call it on one part, and rely on OnDimensionShapeChanged handler to properly manipulate all the symmetry parts, do you get correct behaviour?

No:

I tried a workaround in SmartTank code to update the size of only one of the tanks in the symmetry group to see whether the changes would be applied to it by some part of ProceduralParts, but this only made the problem worse; the tanks ended up different sizes and the attach nodes were still translated way off to the wrong place.

"update the size" meant to call SeekVolume. It doesn't resize the other tanks.

[DRVeyl]

I will investigate this weekend.

[HebaruSan]

I'll also take a look at it again, if the intention is that SeekVolume works across symmetry groups then I could try changing this PR to do that instead.

[DRVeyl]

Normal operation [using the controls to set length/diameter]: when the OnShapeDimensionChanged handler is triggered, KSP has updated all of the values in the symmetry parts already. Iterating through the symmetry counterparts to adjust them is correct.

When SeekVolume is called, scaledField.SetValue(clampedScaledValue, this) does not yet propagate the new value to the symmetry counterparts, so OnShapeDimensionChanged immediately after works as expected for the focused part, but not for the symmetry counterparts. Some time after SeekVolume exits, KSP propagates the change to the counterparts, then invokes OnShapeDimensionChanged again. However, this call sends the current value instead of the previous [at least in 1.8.1], so the code returns immediately with nothing to do. The symmetry counterparts never properly push in either call. The first time you call SeekVolume on parts in symmetry, the symmetry counterparts don't appear to redraw [they redraw but before their values change] and the attach nodes don't move. Their size/values afterward don't reflect the appearance. Subsequent calls look strange and are difficult to diagnose because it never recovers from this de-syncing between the attachment node position, the part dimensions, and the mesh when it was rebuilt.

I'm not certain what is a good idea to resolve. It might be that SeekVolume needs:

scaledField.SetValue(clampedScaledValue, this);
foreach (Part p in part.symmetryCounterparts)
{
    if (FindAbstractShapeModule(p, this) is ProceduralAbstractShape pm)
        scaledField.SetValue(clampedScaledValue, pm);
}
OnShapeDimensionChanged(scaledField, orig);

This feels inelegant, but appears to work. Note that ProceduralShapePill needs the equivalent modification, and FindAbstractShapeModule needs to change from private to support. Test it out, and if this seems to work for you, I can commit and push a new release soon.

[HebaruSan]

Progress! That fixed the length updates, but diameters are still on the wonky side:

[HebaruSan]

This is my current code for updating cylinder diameters, called for all tanks in the symmetry group:

    ProceduralShapeCylinder cyl = part.Modules.GetModule<ProceduralShapeCylinder>();
    float prevDiameter = cyl.diameter;
    cyl.diameter = diameter;
    cyl.OnShapeDimensionChanged(cyl.Fields["diameter"], prevDiameter);

[DRVeyl]

Try moving the last line [cyl.OnShapeDimensionChanged(cyl.Fields["diameter"], prevDiameter);] outside of the loop. That is, loop over all parts in the symmetry group to gather the appropriate PartModule and update its diameter field, but then only call OnShapeDimensionChanged for one of them. Any of them should be fine.

[HebaruSan]

There is no loop, each part runs that code independently in its own PartModule. I'm not sure I can coordinate them that way.

[HebaruSan]

OK, awkwardness of implementation aside, that solution does work!

I'll update this PR with the PP-side changes that are involved...

[DRVeyl]

Some of the awkwardness comes from needing to duplicate the behaviour in stock's UIPartActionFieldItem in protected void SetFieldValue(object newValue). Shortened form:

this.SetSymCounterpartValue(newValue)
this.field.SetValue(newValue, this.field.host);
this.control.onFieldChanged(this.field, oldValue);

That's performed under the hood when you click one of the size changing buttons or move the slider. I think that you could achieve what you're looking for directly with:

ProceduralShapeCylinder cyl = part.Modules.GetModule<ProceduralShapeCylinder>()
(cyl.uiControlEditor.partActionItem as UIPartActionFloatEdit).slider.value = diameter

In theory, you should be able to do that from any part, and you shouldn't have to bother with symmetry counterparts yourself. Let the UIControl system do the work for you. I haven't tried this, I'm just basing it on static analysis.

[HebaruSan]

Where in that code do you specify which field is being changed? Also not having tried it, I'm guessing that wouldn't compile... But yes, I definitely see the advantage in making programmatic changes look more like user changes.

[HebaruSan]

I took a guess regarding specification of the field:

    var f = part.Modules.GetModule<T>()?.Fields[fieldName]?.uiControlEditor.partActionItem as UIPartActionFloatEdit;

... but that's always null. :cry:

[DRVeyl]

Oops, you're correct, I skipped a step. cyl.Fields["diameter"].uiControlEditor

ProceduralShapeCylinder cyl = part.Modules.GetModule<ProceduralShapeCylinder>()
(cyl.Fields["diameter"].uiControlEditor.partActionItem as UIPartActionFloatEdit).slider.value = diameter