linuxgurugamer / SXTContinued

Lack's Stock eXTension
Other
17 stars 26 forks source link

Fixing TweakScale patches #71

Closed Lisias closed 5 years ago

Lisias commented 5 years ago

Fixing TweakScale patches to prevent triggering the Fatal Alert from #34.

This was reported on Forum

There're more patches being applied with wildcards, but they are not borking anything for now. I opted to not touch what's not broken yet.

This also fixes the Osaul Radial Cockpit (SXTOsualRadCockpit) that got terribly small due a patch. Now the cockpit is size compatible with the other Osaul cockpits.

Screen Shot 2019-06-10 at 00 21 49

ncanceill commented 5 years ago

While you are at it:

ncanceill commented 5 years ago

Did you update the PR since my comment? or had you already fixed the inline intake and truck box? (in which case, sorry, I need to learn how to read)

Anyhow, this now fixes everything for me except the three seaplane floats

Moreover, I added percent signs everywhere to avoid duplication, so my patch now reads:

@PART[SXTfloat*]:NEEDS[TweakScale] // 3 parts
{
    %MODULE[TweakScale]
    {
        %type = free
    }
}

However, the three float parts still display two scaling gauges in the PAW and trigger NullRefs in TweakScale (see my comment in TweakScale #41)

@Lisias can you reproduce this? According to my ModuleManager log, SXT/Patches/ModCompatibility/SXT_TweakScale/@PART[SXTfloat*]:NEEDS[TweakScale] is the only patch applied to these parts, and I cannot find anything that would duplicate the TweakScale module...

linuxgurugamer commented 5 years ago

I'm going to wait until this is settled before merging

linuxgurugamer commented 5 years ago

I appreciate all the work you guys are doing. This is an old mod with an old patch, which I don't have time to look at right now. Please make sure I know everyone who contributed to this update, both testing and actually changing the code

Lisias commented 5 years ago

Okie dokie. I will check these fixes on my side, and apply a new pull request.

In the mean time I would advise against using the % on widlcards. This will make the problem impossible to detect. We are talking about unintentional double patching, and this will corrupt the savegames and crafts the same way, but this time in a way that TweakScale cannot detect.

It's better to fix the problem than to patch the symptoms!

Lisias commented 5 years ago

A full discussion about it is on the forum:

https://forum.kerbalspaceprogram.com/index.php?/topic/179030-14-tweakscale-under-lisias-management-2430-2019-0608/&do=findComment&comment=3616634

ncanceill commented 5 years ago

@Lisias thanks for the explanation

I hope we can track down the problem with the seaplane floats

One last thing about the comment you added about the radial cockpit needing type surface instead of type stack: I agree, but why keep it commented? would the changer break existing craft or something?

ncanceill commented 5 years ago

I confirm that the issue with the seaplane floats is not directly due to double patching, because removing the @PART[SXTfloat*]:NEEDS[TweakScale] patch completely remove the Tweakscale module from the part

Moreover, the issue is not due to the wildcard, because it persists when using @PART[SXTfloatFront,SXTfloatMid,SXTfloatOutboard]:NEEDS[TweakScale] to patch instead of the wildcard

So this is likely an issue related to the FireSpitter buoyancy module, either in ModuleManger or in Tweakscale, maybe related to this

I would advise on replacing the other wildcards in this file with the actual part names before merging:

Lisias commented 5 years ago

The problem with getting rid of all wildcard is the need to manually update the TweakScale patch every time you add a new part.

This is a maintenance decision, and is unrelated to the problem at hand - so my reluctance on adding them to this Pull Request, what is exclusively about the https://github.com/net-lisias-ksp/TweakScale/issues/49

About the FSbuoyancy, it's yet another issue as linked above. Also unrelated to the problem at hand.

It worth to mention that workarounds to allow current savegames to go on are already available:

https://github.com/net-lisias-ksp/TweakScale/blob/dev/orthodox/Extras/TweakScale/BreakingParts/SXT.cfg

Lisias commented 5 years ago

Unless any other issue arise, I think this pull request is good for merging.

ncanceill commented 5 years ago

@Lisias I concur!

@linuxgurugamer please merge this at your convenience

In my opinion, the risk of wildcards introducing bugs outweighs the overhead of modifying the patches every now and then, but this decision is not up to me

Lisias commented 5 years ago

I appreciate all the work you guys are doing. This is an old mod with an old patch, which I don't have time to look at right now. Please make sure I know everyone who contributed to this update, both testing and actually changing the code

Last week I was walking over hot coals due some Real Life issues. And that were the good times, I'm seating on them right now. =P

I just forgot to mention the patches here. (sigh) Sorry.

In a way or another, there're a lot of legacy issues that we plain inherited with the Add'Ons. It's just the way it is - people borks. Things goes ugly when these borks goes unnoticed and unfixed.

Well… There're some fewer borks unnoticed nowadays, and they are being fixed. I think we are doing good (besides the bruises).