pellinor0 / TweakScale

Forked from Gaius Goodspeed's Goodspeed Aerospace Part & TweakScale plugin
29 stars 23 forks source link

Stock editor mass display does not update correctly #5

Closed pellinor0 closed 8 years ago

pellinor0 commented 9 years ago

The stock display does not react to scaling. KER and mechjeb show the correct mass.

When I scale a full fuel tank, the content is scaled but the stock display does not change at all. If afterwards I touch the fuel slider, the fuel weight updates but the dry weight does not.

Is there some "ship modified" event that I am supposed to call? If so, how do KER/Mechjeb detect the change?

pellinor0 commented 8 years ago

In the above case detaching and reattaching the part updates the dispay.

Variant: after revert to launch engines (maybe also other parts) are counted with their unscaled mass in the engineer's report. Changing scale or reattaching does not update the display. Again MJ/KER show the right mass.

http://forum.kerbalspaceprogram.com/threads/110899-WIP-TweakScale-Development-Thread?p=2219070&viewfull=1#post2219070

pellinor0 commented 8 years ago

first point fixed by https://github.com/pellinor0/TweakScale/commit/71073d9a6844add8827a73dbe4000be864f5d7b6

The "Variant" of the bug is still present.

m4v commented 8 years ago

I got a report pointing out that recent versions of RCS Build Aid doesn't show the correct mass with TweakScale. RCSBA uses the same method than Engineer's Report for calculate the part mass so this bug is related.

Just so you know, the method that RCSBA uses is

public static float GetTotalMass (this Part part) {
    var mass = part.partInfo.partPrefab.mass;
    return mass + part.GetModuleMass (mass) + part.GetResourceMass ();
}

I think that KER reports the correct values because (last time I checked) it only uses the part.mass value and ignores the IPartMassModifier interface for all parts except stock fairings.

pellinor0 commented 8 years ago

Currently TweakScale writes part.mass directly (just like any other kspField except for cost).

I could move this manipulation into a getModuleMass method, but am not really sure if this will fix more things than it breaks. Are you sure you need to start with the mass of the prefab instead of the part itself? And that the editor behaves like this?

The main bug with the engineer's report was fixed by notifying the game that the vessel has changed. I don't see how this fix could work if the engineer's report never reads part.mass.

My understanding of TweakScale is that it manipulates the base values which others use to make their calculations. So if some other module uses the mass parameter in its getModuleMass method, I'd expect it to do the calculation based on the scaled mass (instead of the prefab mass).

m4v commented 8 years ago

I'm certain that that engineer's report reads the prefab mass and not part.mass. I think the only concern would be breaking KER's readings by changing this, but i think is safe to do the same thing that RealFuels does:

NathanKell pointed out that GetModuleMass should just return part.mass - part.partInfo.partPrefab.mass, see implementation in ModuleFuelTanks.cs So implementing GetModuleMass that returns the delta between part.mass and prefab.mass should make everybody happy, KER will read part.mass like always (except for fairings, then is part.mass + GetModuleMass i think?) and engineer's report and RCSBA will prefab.mass + GetModuleMass() which should just yield the same value in part.mass.

The main bug with the engineer's report was fixed by notifying the game that the vessel has changed. I don't see how this fix could work if the engineer's report never reads part.mass.

My understanding of TweakScale is that it manipulates the base values which others use to make their calculations. So if some other module uses the mass parameter in its getModuleMass method, I'd expect it to do the calculation based on the scaled mass (instead of the prefab mass).

It's possible that I'm wrong here, regrettably I didn't even had the time to verify the bug with RCSBA myself!, and I'm just going by the report I got and my gut feeling of what could be wrong. In the weekend I will have more time for look myself.

m4v commented 8 years ago

The main bug with the engineer's report was fixed by notifying the game that the vessel has changed. I don't see how this fix could work if the engineer's report never reads part.mass.

I checked and the bug isn't fixed, the mass changes but that's just the resource mass, dry mass doesn't change. Testing in a fuel tank without resources shows that mass never changes.

This is consistent with my assumption before, engineer's report reads only the prefab mass + getmodulemass + getresourcemass. Implementing IPartMassModifier interface with the method as described before should be a good bet at fixing this.

pellinor0 commented 8 years ago

Thanks for the analysis, I now report to the IPartMassModifier interface while still scaling part.mass as before.

The issue should now be fixed by https://github.com/pellinor0/TweakScale/commit/8f4d97bc9d0932a1e88a47815b664656275617fe and https://github.com/pellinor0/TweakScale/commit/1756049d7dca17869785b395ad75f130855e7348