blowfishpro / B9PartSwitch

A Kerbal Space Program plugin designed to implement switching of part meshes, resources, and nodes
GNU Lesser General Public License v3.0
50 stars 33 forks source link

Attach node moving should respect scale, rescaleFactor, and tweakscale scaling #118

Closed blowfishpro closed 5 years ago

blowfishpro commented 5 years ago

Need to multiply the position by the scale factor

jsolson commented 5 years ago

Attach node moving is probably definitely not tweakscale aware.

Confirmed. It doesn't work right. I'd expect multiplying by the rescaleFactor to work.

blowfishpro commented 5 years ago

I was less sure about what the behavior with rescaleFactor should be. I guess since normal attach nodes use rescaleFactor this should too?

jsolson commented 5 years ago

This should use the same scale normal nodes use, yes, definitely.

TweakScale aside, when scale = 1 and rescaleFactor = X, node positions should be multiplied by X since we know changing rescaleFactor in a part.cfg does not require adjusting node positions.

I don't know for sure what happens if a plugin sets rescaleFactor at runtime, but since TweakScale appears to need to adjust node positions itself I'd say just the mesh changes. I don't think that's important though because TweakScale does correctly adjust your nodes, it's when B9 does the switch things go wrong.

I don't believe scale = x,y,z inside a MODEL node is relevant here, since that only changes the mesh size, and using that method to rescale a part requires adjusting everything manually.

The Squad LV-T30 part (among others) has a scale not equal to 1. My understanding of scale = Y is it refers to how many meters in the mesh equals one meter in game.

From the LV-T30:

PART
{
    name = liquidEngine
    ...
    scale = 0.1
    node_stack_top = 0.0, 7.21461, 0.0, 0.0, 1.0, 0.0
    node_stack_bottom = 0.0, -7.27403, 0.0, 0.0, -1.0, 0.0
}

Just to keep everybody confused rescaleFactor is not set and defaults to 1.25.

If you use node * scale * rescaleFactor for the positions and check the distance between them it agrees with the in game height of the part (1.8m).

So coming from a part.cfg anyway I would say position = node * scale * rescaleFactor.

There is no scale property of the Part class. Part.scaleFactor looks likely.

Long story short, multiply the positions that we give you in the config by Part.scaleFactor * Part.rescaleFactor when moving nodes and let's see what happens. From what I observe of the current behavior that will work. TweakScale does appear to set rescaleFactor. I don't believe you need additional monitoring of OnWhateverChanged() to catch TweakScale doing it's thing.

blowfishpro commented 5 years ago

scale = x does affect nodes. I'll look into all this.

TweakScale is already setting a parameter on ModuleB9PartSwitch that tells it what the current scaling is (this is used for resource amounts currently). It's just a question of using this when calculating attach node positions.

blowfishpro commented 5 years ago

So it looks like Part.scaleFactor is scale / rescaleFactor from the config, and attach node positions are multiplied by scale * rescaleFactor (assuming that the scale appears before that particular stack node). So as strange as these values are it should be possible to reconstruct the correct ones ...

jsolson commented 5 years ago

I had to see all that for myself to believe it. ~So instead of scaleFactor * rescaleFactor it's scaleFactor * rescaleFactor^2 to translate the node positions in the config to the current part scale. That makes much more sense.~ That was wrong. It works as long as the part hasn't been rescaled. You can not reconstruct the original scale value once rescaleFactor is changed. Unless you cache it or reach into the prefab.

Is scale possibly stored with the mesh in a way you can access it? It must be there because unity understands the units and displays it at the correct size. It's always explained as meters, cm, and mm, and it's values are always 1, 0.1, or 0.01 (they may be the only possible values). NathanKell claims that scale does not rescale the model, just the node positions. rescaleFactor does rescale the model, and the nodes (again).

I'm at a loss for what scale / rescaleFactor is supposed to be.

blowfishpro commented 5 years ago

Why would rescaleFactor be changed? It doesn't look like TweakScale modifies it.

jsolson commented 5 years ago

I'm watching it change. This is the line from TweakScale I linked to previously:

part.rescaleFactor = _prefabPart.rescaleFactor * ScalingFactor.absolute.linear;

This is the code I'm using to watch it:

class Class1 : PartModule
    {
        [KSPField(guiActive = true, isPersistant = false, guiActiveEditor = true, guiName = "Scale")]
        public string scaleDisplay = "";

        [KSPField(guiActive = true, isPersistant = false, guiActiveEditor = true, guiName = "RescaleF")]
        public string rescaleDisplay = "";

        public void FixedUpdate()
        {
            scaleDisplay = part.scaleFactor.ToString();
            rescaleDisplay = part.rescaleFactor.ToString();
        }
    }
blowfishpro commented 5 years ago

Ah duh, because searching code in a forked repository does not work...

Reaching into the prefab shouldn't be a problem, I guess I'll probably just do that.

blowfishpro commented 5 years ago

So it looks like TweakScale uses the prefab's node positions when rescaling, so if you select a subtype with a modified node and then rescale the part stuff will get messed up. Will have to look into how to fix this. Might be a callback I can listen for.

jsolson commented 5 years ago

Didn't you say above you're already getting a parameter set so the resources can be adjusted?

jsolson commented 5 years ago

It appears to do both relative and absolute node positioning. That why I thought it worked, it uses relative positioning when you rescale through the menu. But clone the part and it's screwed up.

This could get ugly. Can you change the scale field to a get/setter so you can respond there? (I'm really not even sure how that gets set). There's this line part.SendEvent("OnPartScaleChanged", data, 0); but CallUpdaters() does not appear to be called from Update() which is one of the places using absolute positioning.

blowfishpro commented 5 years ago

I think I got it working in this build, please let me know if you find a scenario that's still broken

https://blowfish-ksp-b9partswitch-dev-builds.s3.amazonaws.com/builds/ScaleAttachNodeMovers/B9PartSwitch_ScaleAttachNodeMovers_506_v2.5.0-5-g9b4903c.zip

jsolson commented 5 years ago

No good. With 2.5.0 all is well. With this dev build with or without TweakScale installed, it appears the model is being shifted in the opposite direction it should be when the node moves. It may only be shifting in the upwards direction, it's hard to tell. I'm having trouble making sense of what it's doing. If I detach the part the node is in the correct position.

Tested with the Bluedog RD180 engine. Cycling through the models without detaching it moves steadily upwards.

blowfishpro commented 5 years ago

Strange. The part I tested with seemed fine, but I'll look into it on that part.

BTW, you shouldn't need to specify a node position for the "Bare" subtype since it uses the default.

blowfishpro commented 5 years ago

Oh I think I see what I did wrong. Try this build?

https://blowfish-ksp-b9partswitch-dev-builds.s3.amazonaws.com/builds/ScaleAttachNodeMovers/B9PartSwitch_ScaleAttachNodeMovers_507_v2.5.0-5-g871f167.zip

jsolson commented 5 years ago

I think you got it. With Tweakscale and the RD180, I couldn't break it in the editor, cloning works, it survives to launch, revert to launch and reload from a save. Symmetry also appears to work.

BTW, you shouldn't need to specify a node position for the "Bare" subtype since it uses the default.

That's no doubt best practice. I did that without thinking. Edit: Can you clarify, the node should be named but not given a position to use the default position? What happen if a node is not named?

SUBTYPE
{
    name = Flush
    transform = Flush
    NODE
    {
        name = bottom01
        // No position given, uses default position
    }
}
SUBTYPE
{
    name = Flush2
    transform = Flush2
    // No NODE block. What happens to bottom01?
}
SUBTYPE
{
    name = Recessed
    transform = Recessed
    NODE
    {
        name = bottom01
        position = 0.0, 0.3402795, 0.0
    }
}
blowfishpro commented 5 years ago

NODE {} doesn't actually affect whether the node is enabled or not (I mentioned this in the release forum post, I know it's sort of confusing). As it stands, having no NODE {} and one without a position will have the same effect - the node will be put in its default position.