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

New undo/redo implementation #953

Closed
wants to merge 116 commits into from

Conversation

MrStevns
Copy link
Member

@MrStevns MrStevns commented Apr 22, 2018

New backup system using QUndoStack, QUndoCommands and QUndoView as mentioned in #864

  • History window Add Undo Redo Window REQUEST #562
  • Ability to undo/redo:
    • Added keyframes
    • Deleted keyframes
    • Canvas Content (bitmap, vector)
      • Brush
      • Pencil
      • Pen
      • Bucket
      • Move
      • Select
      • Smudge
      • Clear
      • Eraser
      • Polyline
    • Selections
    • Copy
    • Paste
    • Import Image/Sequenece
    • Import Soundfile
    • Delete Layer
      • keys will be returned upon undo
    • New Layer
    • Camera Movement
    • Moved frames
    • Moved Layers
    • Layer rename
    • Camera Properties
  • Deleting a key will scrub to previous keyframe

Most if not everything that is not a preference or setting can be undo/redone now.

It's a huge chunk to review and will require more thorough testing since there a so many ways to make up the undo stack to make it go nuts or crash. I've tested it a lot... but it's difficult to cover all cases although I haven't tested it over a longer period of time, so I hope someone else is willing to give it a proper run before we merge it.

I'll leave in the debug logs for now so you have a better idea where things go wrong (if they do).
You'll know when that happens because the application will crash in almost every case.

I've tried to keep everything as clean, consistent and reusable as possible but I've refactored it a bunch of times so it's possible you'll still encounter leftovers.

closes #864
closes #748
partially solves #187

@MrStevns MrStevns changed the title Add new undo/redo implementation New undo/redo implementation Apr 22, 2018
@chchwy
Copy link
Member

chchwy commented Sep 20, 2018

Will be working on this soon.

@MrStevns
Copy link
Member Author

MrStevns commented Sep 20, 2018 via email

@MrStevns
Copy link
Member Author

MrStevns commented Sep 30, 2018

Alright should be up to date now... unfortunately it seems the way I backup transformation is not good enough anymore because the frame is not updated properly, so i'll have to fix that before we can merge.

@chchwy
Copy link
Member

chchwy commented Oct 2, 2018

@candyface Are you talking about camera transform or selection transform? I may be able to help on this.

@MrStevns
Copy link
Member Author

MrStevns commented Oct 2, 2018

@chchwy I'm talking about "selection transforms". It looks like the issue is not a fault on the backup system though but rather because the keyframe hasn't been updated when it creates a backup. You can verify this by taking a snapshot on mousePress and mouseRelease in Movetool using QImage::save() of the current bitmap image

@MrStevns
Copy link
Member Author

MrStevns commented Oct 2, 2018

@chchwy I've figured out the problem and fixed it locally, so you don't have to look into it. The solution made me realize though that this particular backup element could be made much simpler. I'm out of time for today unfortunately, so it'll be a task for tomorrow

@chchwy
Copy link
Member

chchwy commented Oct 2, 2018

Cool, thanks @candyface

@chchwy chchwy added this to the 0.6.4 milestone Oct 7, 2018
Also fix backup being made when always

+ Refactor some things in beziercurve
 - was required to get a consistent selection state visual...
   it's still not perfect though but making any more changes would take me on a wild ride of refactoring and reworking beziercurve.. which i'd like to avoid in this branch.
* Avoid unnecessary lookups
* Avoid casting continuously
@scribblemaniac scribblemaniac added the 🔷 Major PR (two reviewers when possible) label Sep 13, 2019
Vector:
* Flipping a selection and immediately using select all action
* Leaving tool would sometimes apply the same transform again
* Fix some cases where the curve is selected but vertices are not, thus can't move the selection..
…lement

It occurred to me that it might not be useful to have select frames as a backup element after-all, so i've reverted back to the old behaviour... the only caveat is that the undo/redo state won't restore whether a frame was/is selected but is that important?
@Jose-Moreno Jose-Moreno modified the milestones: 0.6.5, 0.6.6 Jan 15, 2020
@chchwy chchwy modified the milestones: 0.6.6, 0.7 Aug 5, 2020
@J5lx J5lx mentioned this pull request Oct 23, 2020
@J5lx J5lx marked this pull request as draft December 18, 2020 16:02
@scribblemaniac
Copy link
Member

An important step in clearing up our PR backlog is dealing with this PR because some other features are waiting on this. That's not even mentioning that this has some really important improvements to the program as a whole. I have decided to dedicate this Wednesday through to the weekend to getting the ball rolling on this again. Given how sizable the changes are and how this branch is now significantly behind master, I propose the following plan to merge this PR:

  1. First review the new standalone classes. When I'm satisfied with them, I will merge just those files.
  2. Integrate parts of the code needed to backup actions which get backed up by the current system. I will try to write unit tests for these as I add them; in my opinion this is a section where we would really benefit from testing: to make sure the merge goes well, to make sure this code works as it's supposed to, and to make sure we don't have any regressions in this going forward.
  3. Remove existing backup system and integrate parts needed to use new system to backup and restore. All features added so far will be tested before pushing this upstream.
  4. Integrate parts of the code needed to backup actions not previously backed up. These will be tested as they are added.
  5. Do a bunch of free testing, ideally with some other people helping.

I know it's a bit convoluted, but this way we can merge this PR piece-by-piece without breaking anything (theoretically). If I only manage to get through part of this in the time I have, then we'll know exactly where I left off in the merging process, we don't have to worry about merge conflicts for the finished parts anymore or broken builds, and partial testing can be done in the nightly builds before the rest of the work is completed. Additionally after the first step is done, PRs that are held up could start being updated to use the new backup system in parallel to this work if anyone feels so inclined.

@candyface Not sure if it will be necessary yet, but maybe if you have some free time we could set up a meeting to go through some of this together.

@MrStevns
Copy link
Member Author

MrStevns commented Jun 28, 2021

That sounds like a good way to go about it, I will assist you and sure we can setup a meeting . I also have the "undo-redo-rewrite-minimal" branch which is a lot more updated.. as per what we discussed at the last meeting.

It contains a lot less code changes in other classes so bugs may re-appear, eg. I recall having made quite a lot of changes to the vector code to be able to make undo/redo work reliably

I agree though let's review the backupmanager + element classes as standalone, and bring it in and then gradually replace the undo/redo actions, one by one. That's a much easier task to handle than this 5000 loc beast to tame and keep updated...

@MrStevns MrStevns mentioned this pull request Jun 29, 2021
@chchwy chchwy removed this from the 0.7.0 milestone Jul 15, 2021
@MrStevns MrStevns closed this Apr 16, 2024
@MrStevns
Copy link
Member Author

Closed in favour of #1817

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔷 Major PR (two reviewers when possible) Undo
Projects
Status: Discarded
Development

Successfully merging this pull request may close these issues.

Replace the current undo/redo system with QUndoStack Allow to UNDO / REDO "deleting" new frames and layers
5 participants