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?

In D9311#230531, @Brecht Van Lommel (brecht) wrote:

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.

In D9311#231476, @Bastien Montagne (mont29) wrote:

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.