Page MenuHome

Fix T71759: Sculpt/Vertex/Weight Paint Brush Size Gets Undone After Undoing a Stroke.
ClosedPublic

Authored by Bastien Montagne (mont29) on Oct 22 2020, 1:09 PM.
Tags
None
Tokens
"Love" token, awarded by mindinsomnia."100" token, awarded by Frozen_Death_Knight."Love" token, awarded by amonpaike."Love" token, awarded by wo262.

Details

Summary

Ignore brushes and scene's tool settings during undo.

Note that this affects all tool settings, not only sculpt/paint
ones.

Add code preserving scene's toolsettings accross undo.

IDPointers are dealt with special care, we try to keep existing ones for
some (like brushes) when possible.

Note that this only covers ToolSettings, changes done to Brush etc.
other IDs are still undone currently.

Part of T71759.

Diff Detail

Repository
rB Blender

Event Timeline

Bastien Montagne (mont29) requested review of this revision.Oct 22 2020, 1:09 PM
Bastien Montagne (mont29) updated this revision to Diff 30261.

Diff against proper revision.

Note that this is still considered a WIP/proof of concept, so it's more for seeing whether other devs agree on the general idea here. Then it can be finalized, and probably extended at least to Brush IDs too.

Am not especially happy with the changes to scene_foreach_paint, does not exactly help readability, but it's very important imho that we do not keep separated the list of IDs to process (we did that a lot in old code, and it lead to many bugs and hidden issues because an area or the other was often forgotten when adding new ID pointers). Maybe using a macro here could help?

Do we need eSceneForeachProcess, can we add a ID_WALK_CB_* flag instead? I don't think we need to make this scene specific, even though that is the only case using it right now. Logic like the one in scene_foreach_toolsettings_id_pointer_process really belongs together with the rest of the undo code I think, in readfile.c.

I also don't see the need for both SCENE_FOREACH_UNDO_RESTORE and SCENE_FOREACH_UNDO_NO_RESTORE. The distinction is that certain datablock types are UI datablocks (wm, screen, workspace, brush, palette). Both a pointer to and from such datablocks needs different treatment in various parts of the code.

The scene toolsettings then are the odd case, where only part of the datablock is UI data. And I think that could be communicated with an ID_WALK_CB_ flag, which pointers are UI data. But for everything else detecting if it's a UI datablock through IDTypeInfo could be enough?

Do we need eSceneForeachProcess, can we add a ID_WALK_CB_* flag instead? I don't think we need to make this scene specific, even though that is the only case using it right now. Logic like the one in scene_foreach_toolsettings_id_pointer_process really belongs together with the rest of the undo code I think, in readfile.c.

ID_WALK_ flags and whole lib_query code is completely not suited for that specific case, since here we need to loop over pointers of two IDs in parallel.

And on top of that, we are trying to move ID-specific logic out of readfile.c, this part is specific to Scene.

I also don't see the need for both SCENE_FOREACH_UNDO_RESTORE and SCENE_FOREACH_UNDO_NO_RESTORE. The distinction is that certain datablock types are UI datablocks (wm, screen, workspace, brush, palette). Both a pointer to and from such datablocks needs different treatment in various parts of the code.

_UNDO_RESTORE tries to keep the old value of the pointer, i.e. the ID to which we were pointing before the undo step. The other one is just means we use regular pointer generated from the undo step.

ID_WALK_ flags and whole lib_query code is completely not suited for that specific case, since here we need to loop over pointers of two IDs in parallel.

Ok, indeed the two pointers means it has to be different.

And on top of that, we are trying to move ID-specific logic out of readfile.c, this part is specific to Scene.

If we preserve brush datablocks, will it still be scene specific? Won't they need the same mechanism? That's the main reason I think this should be generic.

The logic inside of scene_foreach_toolsettings_id_pointer_process does not seem scene specific, but rather closely related to the way the undo system works.

_UNDO_RESTORE tries to keep the old value of the pointer, i.e. the ID to which we were pointing before the undo step. The other one is just means we use regular pointer generated from the undo step.

Yes, I was suggesting that this could be automatically determined based on the type of pointer. And if we support keeping brush and palette datablocks, maybe this logic gets more complicated and it's better to determine this automatically rather than relying on RESTORE/NO_RESTORE being set correctly?

This is more of a detail though.

WIP work, please ignore.

Bastien Montagne (mont29) planned changes to this revision.Fri, Oct 30, 9:41 AM

Updated against latest master, feature-complete?

Scene's toolsettings, brushes and palettes are now covered.

Diff against proper revision...

It would be great if we could somehow make this more generic, but I don't have any good suggestions so this seems fine.

From testing it seems to work as it should, I couldn't find issues.

This revision is now accepted and ready to land.Mon, Nov 2, 12:45 PM