Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better Undo/Redo can merge two steps into one #206

Closed
numberlesstim opened this issue Feb 25, 2024 · 3 comments
Closed

Better Undo/Redo can merge two steps into one #206

numberlesstim opened this issue Feb 25, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@numberlesstim
Copy link

Steps to reproduce:

  1. Place 2 tanks attached to each other.
  2. Move the second (not root tank) using the "Move" tool.
  3. Undo step 2.
  4. Observe the tank disappearing instead of getting moved to the original place.

Using Redo after this will add the tank back in at the position it was after moving it, so it appears that two steps have been merged.

This "merging" of steps also happens if you change anything in the paw. For example changing fuellevel, open/close a bay, extend panels, change part variant etc.

I did test this with no other mods installed and verified this does not happen without KSPCF.

@numberlesstim
Copy link
Author

Further, if in the above steps you instead move the root tank, it will both remove the second tank and move the root tank back to the original place. Maybe that is a better example.

@gotmachine gotmachine added the bug Something isn't working label Feb 26, 2024
@gotmachine
Copy link
Contributor

gotmachine commented Feb 26, 2024

So...

It's not really a "merging" of steps, but more an oversight of the consequences of inverting the undo/redo logic.

Basically, what the patch does is invert the core undo/redo logic from capturing state after events to capturing it before events.

However, I only moved at which point the state is captured for attach/detach events, not for offset/rotate events, which means you end up with twice the same state captured (in your reproduction steps, you will notice that you have to undo twice at step 3 for step 4 to happen), and the state before offsetting/rotating not being captured at all.

Now that I'm looking at it, there are actually more events that result in the same issue (same repro steps, just change step 2 with the following actions) :

  • Switching between part variants
  • Removing parts from symmetry
  • Adding/removing part actions to action groups, and resetting part actions on the selected part
  • Using the reset button of the stage manager

This will need quite a bit of work to fix them all. Basically, we need to move the call to EditorLogic.SetBackup() before the actions take place (instead of after).

  • Offset/Rotate gizmo
    The backup is created from callbacks called from the gizmo components OnMoveEnd / OnRotateEnd methods, triggered when the gizmo is released. The gizmos do have a On*Start method, but unfortunately they aren't hooked to a callback, and those are generic components that can potentially be used elsewhere. We can still patch those methods, and compare the gizmo instance to the EditorLogic.gizmo* fields.
  • Switching between part variants
    Requires moving the SetBackup() call at the beginning of the ModulePartVariants.onVariantChanged method
    Doesn't work because the field is already set when ModulePartVariants.onVariantChanged, so the changes will be saved to the backup anyway. Alternatively, we patch UIPartActionVariantSelector.SelectVariant()
  • Removing parts from symmetry
    Requires moving the SetBackup() call at the beginning of the Part.RemoveFromSymmetry method
  • Adding/removing part actions to action groups, and resetting part actions on the selected part
    Requires moving the SetBackup() call at the beginning of the EditorActionGroups ResetPart, AddActionToGroup and RemoveActionFromGroup methods
  • Using the reset button of the stage manager
    Requires moving the SetBackup() call at the beginning of the StageManager.Reset method
    Actually after further testing, it seems the backup is already created before the action, so nothing to fix here

gotmachine added a commit that referenced this issue Mar 2, 2024
gotmachine added a commit that referenced this issue Apr 4, 2024
Fixes for incoherent undo/redo behavior introduced by the BetterUndoRedo patch
@gotmachine
Copy link
Contributor

Fix released in 1.35.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants