Rework our ID copying code #51804

New Issue

Bastien Montagne · 2017-06-14T10:44:36+02:00

Bastien Montagne commented

2017-06-14 10:44:36 +02:00

Problem

So, since I've been working on assets and even more on static override, I came over and over across annoying limitations and oddness of our current ID copying code. To summarize current status:

Main datablock copying duplicates ID part itself, inserts it into given Main database, and then call databock type specific copying function that will take care of duplicating sub-data, increment other used IDs usercount, etc.
Then, since we have some needs of other behaviors now and then, we have some specific stuff, one to copy ID without putting it into a Main database, a few specific copying logic for materials, textures etc., used during rendering, that do less 'complete' copying (e.g. by not incrementing other used IDs usercount), etc.
Some datablock types are not copyable at all with generic functions from BKE_library (main one being Scene, but also Sound, VFont, and (but that’s probably not a big issue for now) WindowManager, Screen, Library, and WorkSpace in 2.8).

That somewhat messy and incomplete situation has been annoying me ever since I reworked/factorized other parts of ID management code, but always decided that was low-priority TODO. Now that am working on static override, am hitting issues here more and more, having to add stupid hacks to work around them. And now looks like @Sergey is also facing same kind of issues while working on DEG/CoW.

Proposal

So looks like it’s time to tackle this, and would rather have some consensus on general plan before I start implementing it. I think we need a flexible system allowing to add more copy types/options as need arise, so I would go with flags. I would also try to avoid as much as possible API breakage, due to how many other branches would be impacted, so keeping current API mostly, as wrapper around new one (we can always cleanup later, once transition to 2.8 is finished).

So something like that in main BKE_library api: BKE_id_copy(struct Main *bmain, const struct ID *id, struct ID **newid, const bool test, const int flag);

NULL flag would be same default, full copy as currently with id_copy(). Options would be mix of generic ones, and some specific to some IDs/usages, e.g.:

enum {

/* Generic options (should be handled by all ID types copying). /
LIB_ID_COPY_NO_MAIN = 1 << 0, / Create copy outside of any main database - similar to 'localize' functions of materials etc. /
LIB_ID_COPY_NO_USER_REFCOUNT = 1 << 1, / Do not affect user refcount of datablocks used by copied one. /
LIB_ID_COPY_NO_DEG_TAG = 1 << 2, / Do not tag duplicated ID for update in depsgraph. /
LIB_ID_COPY_NO_ALLOCATE = 1 << 3, / Assume given 'newid' already points to allocated memory for whole datablock (ID + data) - USE WITH CAUTION! */

/* Specific options to some ID types or usages, may be ignored by unrelated ID copying functions. /
LIB_ID_COPY_NO_PROXY_CLEAR = 1 << 8, / Object only, needed by make_local code. /
LIB_ID_COPY_NO_PREVIEW = 1 << 9, / Do not copy preview data, when supported. /
/ ... Most likely more needed ;) */

};

Then add new BKE_material_copy_ex(), BKE_object_copy_ex() etc. that will take same flags as parameter, and behave accordingly.

I think we should also store those copying flags in ID, as runtime data, it has at least two benefits:

We can simplify freeing (since freeing func could check those flags and know whether it should decrement user count, and/or try to remove ID from main…).
More importantly, we can assert at blenfile write time we do not store some 'temp' runtime copy into file (i.e. a copy done without main or usercount)!

Finally, once ready, would aim for this branch to be put into master - but after 2.79 release obviously!

Note that, even though new copying with NO_MAIN/NO_USER_REFCOUNT should behave very similar to them, I do not plan to include existing localize functions in that refactor for now. Ultimately this should be possible, but those have very specific behavior for very specific use case, so would rather not touch them at first.

Things to check

Read again NodeTree localize function, and that one is using a mutex… so was wondering, especially in the DEG/CoW context, whether we may need some 'lock during copy' protection in general? Or should this be handled by DEG itself already?

Problem **** So, since I've been working on assets and even more on static override, I came over and over across annoying limitations and oddness of our current ID copying code. To summarize current status: * Main datablock copying duplicates ID part itself, inserts it into given Main database, and then call databock type specific copying function that will take care of duplicating sub-data, increment other used IDs usercount, etc. * Then, since we have some needs of other behaviors now and then, we have some specific stuff, one to copy ID without putting it into a Main database, a few specific copying logic for materials, textures etc., used during rendering, that do less 'complete' copying (e.g. by not incrementing other used IDs usercount), etc. * Some datablock types are not copyable at all with generic functions from BKE_library (main one being Scene, but also Sound, VFont, and (but that’s probably not a big issue for now) WindowManager, Screen, Library, and WorkSpace in 2.8). That somewhat messy and incomplete situation has been annoying me ever since I reworked/factorized other parts of ID management code, but always decided that was low-priority TODO. Now that am working on static override, am hitting issues here more and more, having to add stupid hacks to work around them. And now looks like @Sergey is also facing same kind of issues while working on DEG/CoW. Proposal **** So looks like it’s time to tackle this, and would rather have some consensus on general plan before I start implementing it. I think we need a flexible system allowing to add more copy types/options as need arise, so I would go with flags. I would also try to avoid as much as possible API breakage, due to how many other branches would be impacted, so keeping current API mostly, as wrapper around new one (we can always cleanup later, once transition to 2.8 is finished). So something like that in main BKE_library api: `BKE_id_copy(struct Main *bmain, const struct ID *id, struct ID **newid, const bool test, const int flag);` NULL `flag` would be same default, full copy as currently with `id_copy()`. Options would be mix of generic ones, and some specific to some IDs/usages, e.g.: ```lang=C enum { ``` /* Generic options (should be handled by all ID types copying). */ LIB_ID_COPY_NO_MAIN = 1 << 0, /* Create copy outside of any main database - similar to 'localize' functions of materials etc. */ LIB_ID_COPY_NO_USER_REFCOUNT = 1 << 1, /* Do not affect user refcount of datablocks used by copied one. */ LIB_ID_COPY_NO_DEG_TAG = 1 << 2, /* Do not tag duplicated ID for update in depsgraph. */ LIB_ID_COPY_NO_ALLOCATE = 1 << 3, /* Assume given 'newid' already points to allocated memory for whole datablock (ID + data) - USE WITH CAUTION! */ ``` ``` /* Specific options to some ID types or usages, may be ignored by unrelated ID copying functions. */ LIB_ID_COPY_NO_PROXY_CLEAR = 1 << 8, /* Object only, needed by make_local code. */ LIB_ID_COPY_NO_PREVIEW = 1 << 9, /* Do not copy preview data, when supported. */ /* ... Most likely more needed ;) */ ``` }; ``` Then add new `BKE_material_copy_ex()`, `BKE_object_copy_ex()` etc. that will take same flags as parameter, and behave accordingly. I think we should also store those copying flags in ID, as runtime data, it has at least two benefits: - We can simplify freeing (since freeing func could check those flags and know whether it should decrement user count, and/or try to remove ID from main…). - More importantly, we can assert at blenfile write time we do not store some 'temp' runtime copy into file (i.e. a copy done without main or usercount)! Finally, once ready, would aim for this branch to be put into master - but after 2.79 release obviously! Note that, even though new copying with NO_MAIN/NO_USER_REFCOUNT should behave very similar to them, I do not plan to include existing `localize` functions in that refactor for now. Ultimately this should be possible, but those have very specific behavior for very specific use case, so would rather not touch them at first. Things to check =========== Read again NodeTree localize function, and that one is using a mutex… so was wondering, especially in the DEG/CoW context, whether we may need some 'lock during copy' protection in general? Or should this be handled by DEG itself already?

Bastien Montagne self-assigned this 2017-06-14 10:44:36 +02:00

Bastien Montagne commented

2017-06-14 10:44:36 +02:00

Changed status to: 'Open'

Bastien Montagne commented

2017-06-14 10:44:36 +02:00

Added subscribers: @Sergey, @mont29, @ideasman42

Sergey Sharybin commented

2017-06-14 11:02:20 +02:00

Some preliminary feedback.

There's a bit more tricky part about DEG/CoW: ideally i'd want to copy datablock content to a pre-allocated memory. This is because memory is allocated at DEG construction time (so we can know pointers to be passed to evaluation functions) but actual content copy happens during evaluation, and happens in threads.

Similar thing would also need to be done for datablock freeing, which is currently also requires bmain and additionally does depsgraph tagging. Btw, DEG tagging should also be probably added to a flags.

Flags approach seems totally fine to me: avoids having fevzillion functions doing similar thing differently or having lots of function arguments which will also become PITA to maintain.

As for localization: surely it's simple to say "we'll just let DEG to handle that" ;) Surely you can construct a separate depsgraph of a single node tree datablock in it on shading/compositing job begin. but tricky thing is: localization has feature of merging data from evaluated tree back to the original one. Depsgrapgh will unlikely to support such feature.

Semi-related thought here: was thinking, shall we get rid of switch statements in lots of places and introduce some sort of ID type registry, where we store what functions are responsible for copy, free, allocate?

Some preliminary feedback. There's a bit more tricky part about DEG/CoW: ideally i'd want to copy datablock content to a pre-allocated memory. This is because memory is allocated at DEG construction time (so we can know pointers to be passed to evaluation functions) but actual content copy happens during evaluation, and happens in threads. Similar thing would also need to be done for datablock freeing, which is currently also requires bmain and additionally does depsgraph tagging. Btw, DEG tagging should also be probably added to a flags. Flags approach seems totally fine to me: avoids having fevzillion functions doing similar thing differently or having lots of function arguments which will also become PITA to maintain. As for localization: surely it's simple to say "we'll just let DEG to handle that" ;) Surely you can construct a separate depsgraph of a single node tree datablock in it on shading/compositing job begin. but tricky thing is: localization has feature of merging data from evaluated tree back to the original one. Depsgrapgh will unlikely to support such feature. Semi-related thought here: was thinking, shall we get rid of switch statements in lots of places and introduce some sort of ID type registry, where we store what functions are responsible for copy, free, allocate?

Bastien Montagne commented

2017-06-14 14:29:39 +02:00

In #51804#440383, @Sergey wrote:
Some preliminary feedback.

There's a bit more tricky part about DEG/CoW: ideally i'd want to copy datablock content to a pre-allocated memory. This is because memory is allocated at DEG construction time (so we can know pointers to be passed to evaluation functions) but actual content copy happens during evaluation, and happens in threads.

Since copy takes a pointer-to-pointer for new (copied) ID, nothing prevents from adding a LIB_ID_COPY_NO_ALLOCATE option and assume given newid already points to valid allocated memory I guess? What would you expect to happen with sub-data (non ID structs), though, I’d assume those should be duplicated as usual (i.e. allocated and copied as needed by BKE_<idtype>_copy() function)?

Similar thing would also need to be done for datablock freeing, which is currently also requires bmain and additionally does depsgraph tagging. Btw, DEG tagging should also be probably added to a flags.

Good point, there should also be an option to avoid DEG tagging.

Flags approach seems totally fine to me: avoids having fevzillion functions doing similar thing differently or having lots of function arguments which will also become PITA to maintain.

As for localization: surely it's simple to say "we'll just let DEG to handle that" ;) Surely you can construct a separate depsgraph of a single node tree datablock in it on shading/compositing job begin. but tricky thing is: localization has feature of merging data from evaluated tree back to the original one. Depsgrapgh will unlikely to support such feature.

Maybe I was not clear enough here: what I mean is that combination of NO_MAIN/NO_USER_COUNT copy should behave and give pretty much same result as what our localize functions produce currently. But I would not bother trying to make those existing localize functions using new API at first (because it's quite intricate and complex code, easy to break, and not crucial to factorize into new system imho).

Semi-related thought here: was thinking, shall we get rid of switch statements in lots of places and introduce some sort of ID type registry, where we store what functions are responsible for copy, free, allocate?

No strong opinion here really… would not consider this high priority anyway ;)

> In #51804#440383, @Sergey wrote: > Some preliminary feedback. > > There's a bit more tricky part about DEG/CoW: ideally i'd want to copy datablock content to a pre-allocated memory. This is because memory is allocated at DEG construction time (so we can know pointers to be passed to evaluation functions) but actual content copy happens during evaluation, and happens in threads. Since copy takes a pointer-to-pointer for new (copied) ID, nothing prevents from adding a `LIB_ID_COPY_NO_ALLOCATE` option and assume given `newid` already points to valid allocated memory I guess? What would you expect to happen with sub-data (non ID structs), though, I’d assume those should be duplicated as usual (i.e. allocated and copied as needed by `BKE_<idtype>_copy()` function)? > Similar thing would also need to be done for datablock freeing, which is currently also requires bmain and additionally does depsgraph tagging. Btw, DEG tagging should also be probably added to a flags. Good point, there should also be an option to avoid DEG tagging. > Flags approach seems totally fine to me: avoids having fevzillion functions doing similar thing differently or having lots of function arguments which will also become PITA to maintain. > > As for localization: surely it's simple to say "we'll just let DEG to handle that" ;) Surely you can construct a separate depsgraph of a single node tree datablock in it on shading/compositing job begin. but tricky thing is: localization has feature of merging data from evaluated tree back to the original one. Depsgrapgh will unlikely to support such feature. Maybe I was not clear enough here: what I mean is that combination of NO_MAIN/NO_USER_COUNT copy should behave and give pretty much same result as what our localize functions produce currently. But I would not bother trying to make those existing localize functions using new API at first (because it's quite intricate and complex code, easy to break, and not crucial to factorize into new system imho). > Semi-related thought here: was thinking, shall we get rid of switch statements in lots of places and introduce some sort of ID type registry, where we store what functions are responsible for copy, free, allocate? No strong opinion here really… would not consider this high priority anyway ;)

Sergey Sharybin commented

2017-06-14 16:39:49 +02:00

What would you expect to happen with sub-data (non ID structs), though, I’d assume those should be duplicated as usual (i.e. allocated and copied as needed by BKE__copy() function)?

That is tricky.. Ideally, DEG CoW should be smart enough about geometry arrays, and have CD_REFERENCE of those to the original data, similar to how DerivedMesh is doing for the case of Mesh without modifiers. On another hand, i'd want things like Pose to be duplicated -- it is light weight anyway.

But that being said, i think it is reasonably to state: LIB_ID_COPY_NO_ALLOCATE only skips ID allocation, but all nested data is allocated and duplicated as usual. If someone needs to do something crazy -- it is all up to him to implement that on his end.

Will keep generic API clear to follow. No need to complicate it just because of some specific crazy corner case.

> What would you expect to happen with sub-data (non ID structs), though, I’d assume those should be duplicated as usual (i.e. allocated and copied as needed by BKE_<idtype>_copy() function)? That is tricky.. Ideally, DEG CoW should be smart enough about geometry arrays, and have `CD_REFERENCE` of those to the original data, similar to how `DerivedMesh` is doing for the case of Mesh without modifiers. On another hand, i'd want things like Pose to be duplicated -- it is light weight anyway. But that being said, i think it is reasonably to state: `LIB_ID_COPY_NO_ALLOCATE` only skips ID allocation, but all nested data is allocated and duplicated as usual. If someone needs to do something crazy -- it is all up to him to implement that on his end. Will keep generic API clear to follow. No need to complicate it just because of some specific crazy corner case.

Bastien Montagne commented

2017-06-14 16:50:43 +02:00

Added subscriber: @dfelinto

Bastien Montagne commented

2017-06-14 16:50:43 +02:00

Will keep generic API clear to follow. No need to complicate it just because of some specific crazy corner case.

Agreed! :)

> Will keep generic API clear to follow. No need to complicate it just because of some specific crazy corner case. Agreed! :)

Bastien Montagne commented

2017-08-31 16:33:53 +02:00

Changed status from 'Open' to: 'Resolved'

Bastien Montagne closed this issue

2017-08-31 16:33:53 +02:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Rework our ID copying code #51804

Things to check