Hmm, we shouldn't really be making undo slower. And this may result in a fairly significant slowdown. Since this is only derived data, it could also be computed lazily (that will probably need a mutex though...). Or we check if we can cache run-time data for each undo step. Not sure how to do that exactly, but I think this may be done in other cases as well. Maybe have a look at rBee3eba902a0b5d5f44b1cb2ff52a3a42cc9e9670.

The basic issue is that the field_inferencing_interface on a node tree is not saved in files.
This wasn't an issue before, because it was only used after it was explicitly recalculated.
However, now we need to use it in the interface to determine if a socket is a field. That
means it shouldn't be explicitly recalculated all the time.

One option would be to calculate the values lazily, guarded by a mutex. But currently I think
that's complexity that shouldn't really be necessary here, especially considering how it relates
to the existing node tree update method.

Another option is to just always update node trees loaded after reloading an undo step.
That was the initial version of this patch. But it is a rather over-the-top solution to this
problem that potentially slows down undo even when this isn't needed.

So, the final option is using this ID caching system, which (I think) is meant to store runtime
data owned by IDs so that it doesn't have to be recalculated when loading an undo step.
It seemed perfect for this situation, until I tried using it. The cache is saved when creating the
undo step, but it is restored with a _different_ node tree, meaning the data can't be found when
restoring it is attempted.

That seems like a fundamental issue, and it leaves me uncertain how the system is supposed
to work. With more details I could probably get this working, but I'm curious if this problem
with undo is even meant to be solved this way.

TL;DR: Think for the time being we should go with the initial version of the patch. It is not a 'nice' solution by any means, but it seems to me that this is the only practical one for now.

I think this is exposing several issues at the same time, both recent (and less recent) ones in node trees and geometry nodes.

Cache storage during undo step uses the pointer of the cached data as part of the key, so if said cached data is re-allocated, it is not matching and on restore step the pointer is simply set to NULL.

I think that if this 'field' data is to be seen as cached data, then it's handling in geometry nodes should be modified (in a nutshell, code should never assume a cache is valid/present, it should use _ensure() API calls or such to rebuild or update the cache as needed).

Fact that nodetrees need calls like ntreeUpdateAllNew in readfile is also a very old very bad thing in the first place.

But all this goes way beyond the scope of this patch...

Thanks for the info @Bastien Montagne (mont29).

Cache storage during undo step uses the pointer of the cached data as part of the key, so if said cached data is re-allocated, it is not matching and on restore step the pointer is simply set to NULL.

Interesting, I think the cache only really works for things with longer lifetimes than the IDs themselves then?

I'm okay with moving forward with the initial version of the patch, but I like to have some longer-term idea of how to improve the situation, so here's an idea:

• Add a Runtime struct pointer on bNodeTree
• Move everything that doesn't need to be saved in files to that
• Move calculating this field inferencing struct to an on-demand system like you suggest.