Refactor and simplify Image (paint) undo code #85797

New Issue

Bastien Montagne · 2021-02-19T17:01:37+01:00

Bastien Montagne commented

2021-02-19 17:01:37 +01:00

At first, image undo type looks similar to Sculpt one, a differential type storing only differences from the previous undo step.

However, attempts to actually make it a proper differential ones have proved to be extremely difficult, mainly due to the overly complicated, under-documented current implementation. It seems to do something at one level, and then compensate backward for it at a lower level, and so on...

Here are some findings (which may or may not be correct/accurate) about current code:

Undo handling is similar to the sculpt one at top level (the image_undosys_step_decode callback), in that it undoes (un-applies) one step 'in the future' (un-applies step n+1 to get step n active).
But unlike sculpt code, during undo it will also process the target step n only if it is the final one.
Internally (uhandle_restore_list), each step stores two states, pre (the UndoImageBuf itself) and post (the UndoImageBuf.post buffer).
When redoing, the post state is always used to restore data.
When undoing, the prestate is always used to restore data, except for the final step, where the post one is used!
post state of step n seems to be shared with pre state of step n+1?

This looks overly complicated, and close to impossible to follow/change behavior, so really think it needs to be refactored and simplified into a fully differential design similar to Sculpt undo type. Or maybe into a fully stateful design, since all the tiles seem to be available in all steps anyway? Would then be similar to memfile or Edit mode undo's.

NOTE: There is no real urgency here, since current code has no known bugs. It's just impossible to maintain and make evolve, and it makes changes to higher-level undo system more risky/complicated since we basically have to deal with a black box here.

At first, image undo type looks similar to Sculpt one, a differential type storing only differences from the previous undo step. However, attempts to actually make it a proper differential ones have proved to be extremely difficult, mainly due to the overly complicated, under-documented current implementation. It seems to do something at one level, and then compensate backward for it at a lower level, and so on... Here are some findings (which may or may not be correct/accurate) about current code: * Undo handling is similar to the sculpt one at top level (the `image_undosys_step_decode` callback), in that it undoes (un-applies) one step 'in the future' (un-applies step `n+1` to get step `n` active). * But unlike sculpt code, during undo it will also process the target step `n` *only if it is the final one*. * Internally (`uhandle_restore_list`), each step stores two states, `pre` (the `UndoImageBuf` itself) and `post` (the `UndoImageBuf.post` buffer). * When redoing, the `post` state is always used to restore data. * When undoing, the `pre`state is always used to restore data, **except** for the final step, where the `post` one is used! * `post` state of step `n` seems to be shared with `pre` state of step `n+1`? This looks overly complicated, and close to impossible to follow/change behavior, so really think it needs to be refactored and simplified into a fully differential design similar to Sculpt undo type. Or maybe into a fully stateful design, since all the tiles seem to be available in all steps anyway? Would then be similar to memfile or Edit mode undo's. NOTE: There is no real urgency here, since current code has no known bugs. It's just impossible to maintain and make evolve, and it makes changes to higher-level undo system more risky/complicated since we basically have to deal with a black box here.

Bastien Montagne commented

2021-02-19 17:01:37 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'