Undo in the editor can unintentionally revert tweaks to part modules
JonnyOThan opened this issue · 7 comments
Repro steps:
- Start a new craft in the editor
- Place a fuel tank
- Change the fuel level in the tank
- Attach another part to the tank
- hit Ctrl-Z to undo
In addition to the second part being deleted, the fuel level in the first tank will be reverted to the full level. This applies to all tweaks to part modules and resources. It seems like maybe the undo state is captured right after a part is placed (i.e. after step 2 above) instead of just before the second part is placed.
Yeah that's something I noticed.
I think there is a usability / performance rationale :
- The amount of captured states is limited. If every small tweak was captured, you would often end up hitting undo dozens of times without getting back to significant changes, or even getting to the bottom of the captured undo states without being able to revert as far as needed.
- Saving the craft induce perceivable stutter with large crafts, this would make interacting with the PAW quite annoying...
- This is compounded by the fact that it's difficult to capture tweaks in a smart way. Most controls don't really have a "on exit" event meaning it might be necessary to capture every value change (especially bad for sliders like resources), in any case this would need to be implemented for every single PAW control, ensuring the state is captured after all callbacks are called. It's also fundamentally not possible to reliably know if tweaking something is inducing a persisted change or not, although one could argue that the vast majority of editor controls are inducing a persisted change.
Oh absolutely; I don't think every tweak should be saved. But I think adding a save state before attaching a new part (and possibly other actions) would solve the problem. Then undoing the attach won't also undo all the tweaks, and then the next undo will undo all the tweaks since the last state. Or the state that gets saved before attaching could replace the previous one.
Overriding the last state before attaching feels like a good solution that wouldn't change the current behavior, every ctrl-z would still result in the last attach action being undone. The main drawback would be that this would double the hiccup lag on attaching, as 2 state save (before and after attaching) are now performed instead of one. Not sure how impactfull this would be in practice.
Right - I also wonder if it would be feasible to then remove the save state from after the attach. We'd probably need to go through and adjust other save state behaviors though.
Gave a look at this, and TBH, it's a bit of a mess.
I initially tried what we were talking about, but that mean a lot of additional captures and I don't like that.
Then I tried to move around when undo states are captured, as the whole thing is kinda implemented backwards...
Instead of capturing the state before events like picking or attaching a part, it capture the state after, and then doesn't restore the last captured state, but the n-1 state. I guess the logic is to have the "redo" state on hand, but that would be better achieved by simply saving the current state before undoing...
Edit : did a bit more fiddling around, and from the perspective of the undo/redo system, inverting the logic actually would work very well. That is, until I realized that the last serialized ship state created by the undo/redo system is what is used to check the available seats for the editor crew assignment. Yay spaghetti mess.
I might have exhausted my will to work on this...
@gotmachine I see you made another commit that addresses the crew manifest issue - is this change ready to go? Perhaps disabled by default?
Edit: Oh I see the PR mentions that change
From my perspective that patch is production ready, and unless I made a mistake somewhere, it shouldn't cause issues.
But as usual, the only way to know for sure is to release it into the wild.