BetterEditorUndoRedo has issues with TweakScale/L

[JonnyOThan]

1. install TweakScale/L and KSPCF with BetterEditorUndoRedo enabled
2. In the editor, place a central fuel tank
3. place another fuel tank attached in 4x symmetry to the side
4. pick up the part placed in symmetry
5. reattach it in a different location

The parts that were placed in symmetry move, but the one you had picked up and put back will pop back to the original location.

I'm not going to investigate this, but I'll disable the BetterEditorUndoRedo patch when TweakScale/L is installed.

https://forum.kerbalspaceprogram.com/topic/204002-18-112-kspcommunityfixes-bugfixes-and-qol-tweaks/?do=findComment&comment=4363621

[Lisias]

Coming from Forum

It’s certainly on KSPCF to not introduce any new issues, which is why I rolled out the workaround quickly. As I said earlier in the thread, that patch isn’t my code (you may be using “your” as a collective though).

I don't care who wrote the code, and you don't should care who wrote the TweakScale™ code involved in the mess neither. All what matters is who is going to fix the code.

Nowadays I'm on a more "comfortable" position because at this time I had wrote or rewrote about 60% [meh… way more…] of that code, so when things goes South there's little to no doubt about who coded the problem (or the trigger to the problem, when the problem is somewhere else). But 5 years ago, 95% of that code were alien to me - and I didn't cared at all.

Maintaining a Open Source project is like marrying and having kids with a woman with kids from a previous relationship - you will raise all of them as they were your own.

I'm not criticising the rollback neither, Kraken knows how many times I did it in the past. I'm telling you that I think you shouldn't had stopped there.

Based on your description though, where would AttachedOnEditor normally refresh its data if you simply pick up and reattach a part?

Because TweakScale doesn't knows who is screwing with the Attachments, neither when - not to mention why. All what it's known is that by restoring the attachment points on the PartModule's Life Cycle, and refreshing them when they change, a lot of misbehaviours involving PartModuleVariant just cease to happen.

None of the code paths you mention would be entered in that case, unless I am mistaken (not currently at a PC to check).

Now I'm worried. This might mean that the BetterEditorUndoRedo may be desyncing something else, completely unrelated to both of us, that by itself is not issuing, or issuing whatever triggers that events on a Stock installation on a time where things is not being expected.

I will need to track down carefully the order of the events from AttachedOnEditor with and without BetterEditorUndoRedo activated and see what's changing so. This is a pretty cumbersome and boring task… (sigh).

I will be back to you later on the Week End.

[JonnyOThan]

Now I'm worried. This might mean...

It can mean a lot of things. I think the most likely are:

• You missed some call to the refresh logic when you listed those, and there's some additional pathway that would be called
• I'm mistaken about whether those pathways would be activated when picking up and reattaching a part

A breakpoint in the AttachedOnEditor code should make it pretty clear in those cases.

[Lisias]

You missed some call to the refresh logic when you listed those, and there's some additional pathway that would be called

Unlikely. I cross referenced the functions that call the one that sends the KSPEvent, and in code I write there's only one place things are done - I do some stretching now and then to guarantee that whatever something is being made, there's only one place where it's being made, so there's only one place to look when something involving the code happens.

But it doesn't hurt to check it again. I will do it after Day Job©.

I'm mistaken about whether those pathways would be activated when picking up and reattaching a part

It's more than 3 years since I wrote AttachedOnEditor, in the KSP 1.9 era. The misbehaviour that leaded me to write it started to bite me on KSP 1.9.

Since them, a lot of water passed thought below the bridge, and I don't remember anymore all the details of the research I did to come to this solution.

The information I gave you may not be 100% accurate neither.

Another possibility, and this is what's really worrying me, is me, by pure chance (and a huge and enourmous luck - of lack of), ended up hitting a sweet spot in which things happen by accident and the presence of the BetterEditorUndoRedo shifted a bit that sweet spot and AttachedOnEditor got screwed by that. It's possible, but them we would need to explain because it's happening just now with BetterEditorUndoRedo.

Flimsy sweet spots usually ran away pretty frequently, not only under a specific configuration like this one.

Of course, there's the chance of this happening on the field and nobody telling me about - only 1 in 26 unhappy customers complain after all, and perhaps I'm really that (un)lucky mate that managed to hit the smallest bull's eye of the Sweet Spot Land.

A breakpoint in the AttachedOnEditor code should make it pretty clear in those cases.

Breakpoints help to pinpoint algorithm problems. This doesn't appears to be one of them, because both things work fine separately.

We are handling something not happening, or happening on the wrong time. A breakpoint will mess up the diagnosis, because it will change dramatically how things are happening in the Editor's guts. Literally, this is like quantum physics - you destroy the scenario you are trying to measure by measuring it before being able to do the measure.

Logging messages with accurate time stamps will help me on this one - it's not enough to detect if something is being called or not, I need to check WHEN it's being called too. And what was called before, and what will be called after. And how many milliseconds between them, to tell you the true - even this can be the source of the problem, in case I have that sweet spot problem.

Any shift on that order (or perhaps the timings) will pinpoint a change on whatever is calling TS that by itself is issuing (or not) the KSPEvent in question.

We are handling asynchronous events here, where anything can be happening at any time. Including nothing.

[JonnyOThan]

Breakpoints help to pinpoint algorithm problems

Well no, the two hypotheses I stated have to do with how a piece of code is entered when a certain action is performed. If you put a breakpoint in there and then do the action, you'll see the callstack - or the breakpoint won't be hit. That will tell us with minimal effort whether our understanding of the code is correct.

Also, I think you have some very incorrect ideas about how Unity works as far as timing and async code or whatever, but I'm not going to spend time debating it. If you want to spend time pursuing those ideas, I can't stop you.

[Lisias]

Well no, the two hypotheses I stated have to do with how a piece of code is entered when a certain action is performed. If you put a breakpoint in there and then do the action, you'll see the callstack - or the breakpoint won't be hit. That will tell us with minimal effort whether our understanding of the code is correct.

Ah, yes. The callstack - nice idea, indeed. I will log the callstack of the calls too. This will be really useful.

Also, I think you have some very incorrect ideas about how Unity works as far as timing and async code or whatever, but I'm not going to spend time debating it. If you want to spend time pursuing those ideas, I can't stop you.

I have evidences of multirheading screwing up things on the initialisation - and it must be a thread, because otherwise that specific misbehaviour would not happen. Two atomic (from the C# point of view) statements just can't be executed simultaneously on co-routines. (Of course I may be wrong about the atomicity, but until I get evidences in contrary, this theory is the best one that fits the behaviour).

I also have evidences of co-routines trying to sync between them by brute force. Something changes on the wrong place and breaks the original flux of execution, and things start to happen out of the order.

This is what happening here? I don't have the slightest clue. But convincing myself this is not what's happening now will help me to redirect my attention to other places that I'm not being able to foresee now, as I still think this may be happening and the apparent misbehaviour fits.

Cognition bias is a thing, and I'm susceptible to it as anyone else. Removing the bias (assuming it's a bias) will help me to redirect efforts to better places.

[gotmachine]

This happen because the AttachedOnEditor module is relying on the editor making backups after attaching / detaching a part, which consequently trigger a call to AttachedOnEditor.OnSave().

By design, the BetterEditorUndoRedo patch whole purpose is to invert the backup logic to capture state before attaching/detaching, which consequently cause the AttachedOnEditor module to capture an outdated node state, and since it blindly force restore it on every first update (the module is disabling itself after the first update, but KSP re-enable it when attaching), this is causing the problematic behavior.

Messing around with attach nodes like this is questionable at best. In any case it's a nuclear option relying on an extremely fragile chain of mostly unrelated events, and whatever issue this is trying to handle could with no doubt be handled without such high risks of causing side effects (not to mention the overhead of spamming an additional module on every part in the game). If all of the 90+ KSPCF bug fixes and patches were written with such disregard for preserving the stock behavior, by now there would be a never ending list of issues with most plugins in the ecosystem.

This being said, disabling the KSPCF patch when AttachedOnEditor is detected feels like the least worst solution for end users.
@Lisias Let us know if you manage to fix it on your end, so we can re-enable this KSPCF patch in the interest of our common users.

[Lisias]

Okey, then. I will further pursue this issue on KSP-Recall (as it's clear that it needs some more robustness).

Thank you for the information. I appreciate it.