blender
/
blender
137
416
Fork 599
Code Issues 6.3k Pull Requests 767 Projects 19 Wiki Activity

Modifiers: add unique modifier identifiers #117347

Merged
Jacques Lucke merged 17 commits from JacquesLucke/blender:modifier-identifier into main 2024-02-06 17:10:48 +01:00
Conversation 20 Commits 17 Files Changed 16 +116 -3
Jacques Lucke commented 2024-01-19 20:04:57 +01:00
Member
Copy Link

This adds a new ModifierData.persistent_uid integer property with the following properties:

  • It's unique within the object.
  • Match between the original and evaluated object.
  • Stable across Blender sessions.
  • Stable across renames and reorderings of modifiers.

Potential use-cases:

  • Everywhere where we currently use the name as identifier. For example ModifierComputeContext and ModifierViewerPathElem.
  • Can be used as part of a key in IDCacheKey to support caches that stay in-tact across undo steps.
  • Can be stored in the SpaceNode to identify the modifier whose geometry node tree is currently pinned (this could use the name currently, but that hasn't been implemented yet).

This new identifier has some overlap with ModifierData.session_uid, but there are some differences:

  • session_uid is unique within the entire Blender session (except for duplicates between the original and evaluated data blocks).
  • session_uid is not stable across Blender sessions.

Especially due to the first difference, it's not immediately obvious that the new persistent_uid can fulfill all use-cases of the existing session_uuid. My guess is that the persistent_uid can replace session_uid though. That can be done as a separate cleanup step.

Unfortunately, there is not a single place where modifiers are added to objects currently. Therefore, there are quite a few places that need to ensure valid identifiers. I tried to catch all the places, but it's hard to be sure. Therefore, I added an assert in object_copy_data that checks if all identifiers are valid. This way, we should be notified relatively quickly if issues are caused by invalid identifiers.

This adds a new `ModifierData.persistent_uid` integer property with the following properties: * It's unique within the object. * Match between the original and evaluated object. * Stable across Blender sessions. * Stable across renames and reorderings of modifiers. Potential use-cases: * Everywhere where we currently use the name as identifier. For example `ModifierComputeContext` and `ModifierViewerPathElem`. * Can be used as part of a key in `IDCacheKey` to support caches that stay in-tact across undo steps. * Can be stored in the `SpaceNode` to identify the modifier whose geometry node tree is currently pinned (this could use the name currently, but that hasn't been implemented yet). This new identifier has some overlap with `ModifierData.session_uid`, but there are some differences: * `session_uid` is unique within the entire Blender session (except for duplicates between the original and evaluated data blocks). * `session_uid` is not stable across Blender sessions. Especially due to the first difference, it's not immediately obvious that the new `persistent_uid` can fulfill all use-cases of the existing `session_uuid`. My guess is that the `persistent_uid` can replace `session_uid` though. That can be done as a separate cleanup step. Unfortunately, there is not a single place where modifiers are added to objects currently. Therefore, there are quite a few places that need to ensure valid identifiers. I tried to catch all the places, but it's hard to be sure. Therefore, I added an assert in `object_copy_data` that checks if all identifiers are valid. This way, we should be notified relatively quickly if issues are caused by invalid identifiers.
Jacques Lucke added 1 commit 2024-01-19 20:05:10 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
f4321da89f add ModifierData.identifier
Jacques Lucke commented 2024-01-19 20:07:00 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Jacques Lucke requested review from Brecht Van Lommel 2024-01-19 20:07:18 +01:00
Jacques Lucke requested review from Sergey Sharybin 2024-01-19 20:07:19 +01:00
Jacques Lucke requested review from Hans Goudey 2024-01-19 20:07:19 +01:00
Hans Goudey commented 2024-01-19 20:22:45 +01:00
Member
Copy Link

What do you think about replacing code like BLI_addtail(&ob->modifiers, md); with a function BKE_object_modifier_add? Or possibly creating a modifier and adding it to an object in a single step: BKE_object_modifier_new. Those might be a bit simpler than trying to ensure a proper identifier after adding to the list manually.

What do you think about replacing code like `BLI_addtail(&ob->modifiers, md);` with a function `BKE_object_modifier_add`? Or possibly creating a modifier and adding it to an object in a single step: `BKE_object_modifier_new`. Those might be a bit simpler than trying to ensure a proper identifier after adding to the list manually.
Hans Goudey reviewed 2024-01-19 20:24:06 +01:00
source/blender/makesdna/DNA_modifier_types.h Outdated
@ -135,0 +135,4 @@
/**
* Uniquely identifies the modifier within the object. This identifier is stable across Blender
* sessions. Modifiers on the original and corresponding evaluated object have matching
* identifiers. The identifier stays attached to the modifier if it is renamed or moved in the
Hans Goudey commented 2024-01-19 20:23:58 +01:00
Member
Copy Link

attached to the modifier -> the same

`attached to the modifier` -> `the same`
JacquesLucke marked this conversation as resolved
Jacques Lucke commented 2024-01-19 20:38:05 +01:00
Author
Member
Copy Link

I thought about both cases. Both can improve the situation, but don't provide a perfect abstraction yet (modifiers are often created in a context where the object is not available, and are sometimes also not added at the end). Also I found it somewhat annoying to have both ED_object_modifier_add and BKE_object_modifier_add. Sometimes we also want to add modifiers without changing the identifiers (e.g. when making a copy for the depsgraph).

Generally, this also wouldn't simplify finding all the places where unique identifiers have to be created. It just moves the problem to a different patch.

The problem is also similar to ensuring unique names. But it's not exactly the same. Sometimes we have to initialize the identifier, but not the name (e.g. we know that there is only one modifier). And sometimes the name has to be made unique, but the identifier does not have to be initialized (e.g. after renaming).

Overall, having either BKE_object_modifier_add and/or BKE_object_modifier_new didn't make it easier for me to make this patch. Would still be nice if we could have a better API for that at some point.

I thought about both cases. Both can improve the situation, but don't provide a perfect abstraction yet (modifiers are often created in a context where the object is not available, and are sometimes also not added at the end). Also I found it somewhat annoying to have both `ED_object_modifier_add` and `BKE_object_modifier_add`. Sometimes we also want to add modifiers without changing the identifiers (e.g. when making a copy for the depsgraph). Generally, this also wouldn't simplify finding all the places where unique identifiers have to be created. It just moves the problem to a different patch. The problem is also similar to ensuring unique names. But it's not exactly the same. Sometimes we have to initialize the identifier, but not the name (e.g. we know that there is only one modifier). And sometimes the name has to be made unique, but the identifier does not have to be initialized (e.g. after renaming). Overall, having either `BKE_object_modifier_add` and/or `BKE_object_modifier_new` didn't make it easier for me to make this patch. Would still be nice if we could have a better API for that at some point.
Jacques Lucke added 1 commit 2024-01-19 21:14:05 +01:00
Sergey Sharybin commented 2024-01-22 12:17:17 +01:00
Owner
Copy Link

Having two conflicting/overlapping identification systems will lead to a confusion. From the use-cases I am aware of having uniqueness within an object is enough. To my knowledge we do not use the modifier_uuid for really globally identifying the modifier. The main intention was to be able to match modifiers between original and evaluated context.

Having a global counter was a simple and cheap way to achieve the desired behavior.

If we somehow can guarantee that removal of modifier and addition of a new modifier does not re-use old modifier ID then we can quite easily remove the session_uuid.

Thing I am not sure about, is how this interferes (if at all) with the current state of overrides. The immediate questions are:

  • Does it get in a way of the current state and possible future plans w.r.t support of overriding modifiers (add, remove)
  • With the re-sync process, and linking data between different files point of view. If the currently visible modifier is name-based then it is something that artists can understand. But if the re-sync process chances IDs then it becomes less obvious why in an editor you don't see modifier anymore.
Having two conflicting/overlapping identification systems will lead to a confusion. From the use-cases I am aware of having uniqueness within an object is enough. To my knowledge we do not use the `modifier_uuid` for really globally identifying the modifier. The main intention was to be able to match modifiers between original and evaluated context. Having a global counter was a simple and cheap way to achieve the desired behavior. If we somehow can guarantee that removal of modifier and addition of a new modifier does not re-use old modifier ID then we can quite easily remove the `session_uuid`. Thing I am not sure about, is how this interferes (if at all) with the current state of overrides. The immediate questions are: * Does it get in a way of the current state and possible future plans w.r.t support of overriding modifiers (add, remove) * With the re-sync process, and linking data between different files point of view. If the currently visible modifier is name-based then it is something that artists can understand. But if the re-sync process chances IDs then it becomes less obvious why in an editor you don't see modifier anymore.
Brecht Van Lommel reviewed 2024-01-22 12:35:14 +01:00
Brecht Van Lommel left a comment
Owner
Copy Link

This indeed looks like it can replace session_uuid in the modifier.

This indeed looks like it can replace `session_uuid` in the modifier.
source/blender/blenkernel/intern/modifier.cc Outdated
@ -1022,1 +1034,4 @@
void BKE_modifiers_identifier_init(const Object &object, ModifierData &md)
{
blender::RandomNumberGenerator rng = blender::RandomNumberGenerator::from_random_seed();
Brecht Van Lommel commented 2024-01-22 12:29:20 +01:00
Owner
Copy Link

I would not use a random number generator for this. It makes automated tests and debugging harder if results are not the same from run to run.

Maybe hash the modifier name, and then if that still conflicts it can be the seed for RNG.

I would not use a random number generator for this. It makes automated tests and debugging harder if results are not the same from run to run. Maybe hash the modifier name, and then if that still conflicts it can be the seed for RNG.
JacquesLucke marked this conversation as resolved
Jacques Lucke added 2 commits 2024-01-22 14:02:21 +01:00
Jacques Lucke commented 2024-01-22 14:15:53 +01:00
Author
Member
Copy Link

If we somehow can guarantee that removal of modifier and addition of a new modifier does not re-use old modifier ID then we can quite easily remove the session_uuid.

That is not guaranteed yet. I can think of a few ways to guarantee this with different trade-offs:

  • Store a next_modifier_identifier on the Object which is strictly increasing. Has a problem with overflows, but that's probably theoretical.
  • Remember all modifier identifiers that have been removed in the current session in some set and forbid using those identifiers again in the current session. The makes modifier identifiers potentially less deterministic of modifiers are removed/created on different threads. This could potentially be mitigated a bit by storing this set per Object session uid.

Thing I am not sure about, is how this interferes (if at all) with the current state of overrides.

Not really something I'm able to answer yet unfortunately. Maybe @mont29 can provide some insight.

But if the re-sync process chances IDs then it becomes less obvious why in an editor you don't see modifier anymore.

If it stays semantically the same modifier, it should keep the same identifier of course. The other way seems more confusing generally: that renaming could lead a the node editor to loose track of which modifier it's using.

So you both would prefer if ModifierData.session_uid is removed by this patch directly? Fine with me, just wondering since that mixes more changes into the patch that aren't strictly necessary..

> If we somehow can guarantee that removal of modifier and addition of a new modifier does not re-use old modifier ID then we can quite easily remove the session_uuid. That is not guaranteed yet. I can think of a few ways to guarantee this with different trade-offs: * Store a `next_modifier_identifier` on the `Object` which is strictly increasing. Has a problem with overflows, but that's probably theoretical. * Remember all modifier identifiers that have been removed in the current session in some set and forbid using those identifiers again in the current session. The makes modifier identifiers potentially less deterministic of modifiers are removed/created on different threads. This could potentially be mitigated a bit by storing this set per Object session uid. > Thing I am not sure about, is how this interferes (if at all) with the current state of overrides. Not really something I'm able to answer yet unfortunately. Maybe @mont29 can provide some insight. > But if the re-sync process chances IDs then it becomes less obvious why in an editor you don't see modifier anymore. If it stays semantically the same modifier, it should keep the same identifier of course. The other way seems more confusing generally: that renaming could lead a the node editor to loose track of which modifier it's using. --- So you both would prefer if `ModifierData.session_uid` is removed by this patch directly? Fine with me, just wondering since that mixes more changes into the patch that aren't strictly necessary..
Sergey Sharybin commented 2024-01-22 17:10:15 +01:00
Owner
Copy Link

Fine with me, just wondering since that mixes more changes into the patch that aren't strictly necessary.

Mixing both addition of identifier and removal of session_uuid might indeed make PR review less readable.

The important part to me is to not have identifier and session_uuid co-existing for a long period of time. Having something that is more generally useful and can replace session_uuid on modifier is something that makes me more excited as an opposite of an extra similar field with potential use-cases (which is also not very clear to me whether those use-cases is something you've already run into and need solution, or is it more of a theoretical possibility for the future).

Store a next_modifier_identifier on the Object which is strictly increasing. Has a problem with overflows, but that's probably theoretical.

This is already theoretically a problem, which probably is also worse because all objects are using the same global counter.

> Fine with me, just wondering since that mixes more changes into the patch that aren't strictly necessary. Mixing both addition of `identifier` and removal of `session_uuid` might indeed make PR review less readable. The important part to me is to not have `identifier` and `session_uuid` co-existing for a long period of time. Having something that is more generally useful and can replace `session_uuid` on modifier is something that makes me more excited as an opposite of an extra similar field with potential use-cases (which is also not very clear to me whether those use-cases is something you've already run into and need solution, or is it more of a theoretical possibility for the future). > Store a `next_modifier_identifier` on the `Object` which is strictly increasing. Has a problem with overflows, but that's probably theoretical. This is already theoretically a problem, which probably is also worse because all objects are using the same global counter.
Brecht Van Lommel commented 2024-01-22 17:23:20 +01:00
Owner
Copy Link

About the name would, what do you think about persistent_uid? My first guess would be that identifier is a string, and that term is already used for various purposes in Blender as a string.

If this gets used in more places, it seems nice to me to have a name that is easy to search, and recognize as the same concept on multiple data structures.

About the name would, what do you think about `persistent_uid`? My first guess would be that `identifier` is a string, and that term is already used for various purposes in Blender as a string. If this gets used in more places, it seems nice to me to have a name that is easy to search, and recognize as the same concept on multiple data structures.
Jacques Lucke commented 2024-01-22 17:28:58 +01:00
Author
Member
Copy Link

The important part to me is to not have identifier and session_uuid co-existing for a long period of time.

If it's clear that we want to replace session_uid with this new identifier afterwards, I can definitely work on this once this patch is merged. Should be relatively straight forward I hope.

This is already theoretically a problem, which probably is also worse because all objects are using the same global counter.

Not sure if it's worse. Previously, you could at least restart Blender and the counter would reset. That's not possible if it's part of DNA.

About the name would, what do you think about persistent_uid? My first guess would be that identifier is a string, and that term is already used for various purposes in Blender as a string.

Sounds good. Note that we have something similar as bNode.identifier. I'm fine with renaming that to persistent_uid as well though.

> The important part to me is to not have identifier and session_uuid co-existing for a long period of time. If it's clear that we want to replace `session_uid` with this new identifier afterwards, I can definitely work on this once this patch is merged. Should be relatively straight forward I hope. > This is already theoretically a problem, which probably is also worse because all objects are using the same global counter. Not sure if it's worse. Previously, you could at least restart Blender and the counter would reset. That's not possible if it's part of DNA. > About the name would, what do you think about persistent_uid? My first guess would be that identifier is a string, and that term is already used for various purposes in Blender as a string. Sounds good. Note that we have something similar as `bNode.identifier`. I'm fine with renaming that to `persistent_uid` as well though.
Jacques Lucke added 2 commits 2024-01-22 17:59:54 +01:00
Brecht Van Lommel approved these changes 2024-01-22 18:03:08 +01:00
source/blender/blenkernel/BKE_modifier.hh Outdated
@ -477,0 +480,4 @@
/**
* Updates `md.persistent_uid` so that it is a valid identifier (>=1) and is unique in the object.
*/
void BKE_modifiers_identifier_init(const Object &object, ModifierData &md);
Brecht Van Lommel commented 2024-01-22 18:02:42 +01:00
Owner
Copy Link

Rename these functions too?

Rename these functions too?
JacquesLucke marked this conversation as resolved
Jacques Lucke added 1 commit 2024-01-22 18:39:47 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
3a7c46793a rename function to BKE_modifiers_persistent_uid_init
Jacques Lucke commented 2024-01-22 18:40:34 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Jacques Lucke requested review from Bastien Montagne 2024-01-22 18:40:46 +01:00
Hans Goudey approved these changes 2024-01-22 18:53:20 +01:00
Sergey Sharybin approved these changes 2024-01-23 09:39:51 +01:00
source/blender/blenkernel/BKE_modifier.hh Outdated
@ -477,0 +485,4 @@
* Returns true when all the modifier identifiers are positive and unique. This should generally be
* true and should only be used by asserts.
*/
bool BKE_modifiers_identifers_are_valid(const Object &object);
Sergey Sharybin commented 2024-01-23 09:36:56 +01:00
Owner
Copy Link

Should this be BKE_modifiers_persistent_uids_are_valid ?

Should this be `BKE_modifiers_persistent_uids_are_valid` ?
JacquesLucke marked this conversation as resolved
Jacques Lucke added 2 commits 2024-01-23 10:14:30 +01:00
Bastien Montagne requested changes 2024-01-23 15:37:42 +01:00
Bastien Montagne left a comment
Owner
Copy Link

I think there is indeed an issue with liboverrides...

To be clear, expected behavior for liboverrides would be that both the local 'copies' of the modifiers and their source modifiers from the linked reference object would have the same persistent_uid, unless am mistaken? That's what the current PR code seems to be doing anyway.

The problem is that local modifiers can be added to an override, which do not exist in the linked data. Once linked data changes (and modifiers get added to the linked objects), there will be persistent_uid collisions...

This gets even more likely with the recursive liboverrides case (where the current local liboverride is created based on linked data that is already overriding some other linked data, etc.).

I have no idea about how to solve such collision in case it would happen, that would not involve modifying some of these persistent UIDs. I can see at least two ideas to avoid or reduce the likeliness of such collisions though:

Ranges of persistent_uids

This solution should ensure that there are never UIDs collisions in liboverrides, between local modifiers, and these coming from the linked reference data.

Assuming that there will be no practical situations where the whole positive int32_t range of values will be used for an object, and that (e.g.) there will never be more than 128 levels of liboverrides (current code somewhat clamp it at 100 max, see lib_override_sort_libraries_func), we could reserve the highest 7 bits (not counting the sign bit) to define some sort of 'liboverride UID space' for all persistent_uid created for an object:

  • Purely local objects would use the value 0;
  • First level of liboverrides would use the level 1.
  • etc.

This value would be a runtime one defined by the readfile (and liboverride) code.

This would ensure that we never have UIDs collisions in liboverrides, within these boundaries:

  • There are less the ~16M (2^24) modifiers per object
  • There are less than 128 levels of recursive liboverrides.

Generate the seed of the RNG from more data.

This solution is likely trivial to implement, but would only strongly reduce the likeliness of collision, not entirely rule it out.

The idea is to initialize the RNG with a seed generated from more data than just the modifier name. For the liboverride case, it could be e.g. the library filepath of the linked reference object ID.

I think there is indeed an issue with liboverrides... To be clear, expected behavior for liboverrides would be that both the local 'copies' of the modifiers and their source modifiers from the linked reference object would have the same `persistent_uid`, unless am mistaken? That's what the current PR code seems to be doing anyway. The problem is that local modifiers can be added to an override, which do not exist in the linked data. Once linked data changes (and modifiers get added to the linked objects), there will be `persistent_uid` collisions... This gets even more likely with the recursive liboverrides case (where the current local liboverride is created based on linked data that is already overriding some other linked data, etc.). I have no idea about how to solve such collision in case it would happen, that would not involve modifying some of these persistent UIDs. I can see at least two ideas to avoid or reduce the likeliness of such collisions though: ### Ranges of `persistent_uid`s This solution should ensure that there are never UIDs collisions in liboverrides, between local modifiers, and these coming from the linked reference data. Assuming that there will be no practical situations where the whole positive `int32_t` range of values will be used for an object, and that (e.g.) there will never be more than 128 levels of liboverrides (current code somewhat clamp it at 100 max, see `lib_override_sort_libraries_func`), we could reserve the highest 7 bits (not counting the sign bit) to define some sort of 'liboverride UID space' for all `persistent_uid` created for an object: * Purely local objects would use the value `0`; * First level of liboverrides would use the level `1`. * etc. This value would be a runtime one defined by the readfile (and liboverride) code. This would ensure that we never have UIDs collisions in liboverrides, within these boundaries: * There are less the ~16M (2^24) modifiers per object * There are less than 128 levels of recursive liboverrides. ### Generate the seed of the RNG from more data. This solution is likely trivial to implement, but would only strongly reduce the likeliness of collision, not entirely rule it out. The idea is to initialize the RNG with a seed generated from more data than just the modifier name. For the liboverride case, it could be e.g. the library filepath of the linked reference object ID.
source/blender/blenkernel/BKE_object.hh Outdated
@ -115,3 +115,3 @@
*
* \warning *Does not* clear modifier stack and related data (particle systems, soft-body,
* etc.) in `ob_dst`, if needed calling code must do it.
* etc.) in `ob_dst`, if needed calling code must do it. The caller is responsible for ensuring the
Bastien Montagne commented 2024-01-23 14:35:59 +01:00
Owner
Copy Link

The caller is also responsible...

`The caller is also responsible...`
JacquesLucke marked this conversation as resolved
source/blender/blenloader/intern/versioning_400.cc Outdated
@ -2669,0 +2670,4 @@
LISTBASE_FOREACH (Object *, ob, &bmain->objects) {
int uid = 1;
LISTBASE_FOREACH (ModifierData *, md, &ob->modifiers) {
md->persistent_uid = uid++;
Bastien Montagne commented 2024-01-23 14:33:44 +01:00
Owner
Copy Link

These versioning-generated persistent_uid won't be persistent for linked data, and may (will) change from reading to reading when the linked data is modified.

Am not sure how much of a problem this will be in practice, but I think it should at least be documented as a comment here?

These versioning-generated `persistent_uid` won't be persistent for linked data, and may (will) change from reading to reading when the linked data is modified. Am not sure how much of a problem this will be in practice, but I think it should at least be documented as a comment here?
JacquesLucke marked this conversation as resolved
Jacques Lucke commented 2024-01-23 15:52:06 +01:00
Author
Member
Copy Link

Thanks for the feedback. Seems reasonable overall, will have to think about it a bit. I do wonder, does that mean that modifier names are also not unique in such a situation?

Thanks for the feedback. Seems reasonable overall, will have to think about it a bit. I do wonder, does that mean that modifier names are also not unique in such a situation?
Bastien Montagne commented 2024-01-23 16:34:37 +01:00
Owner
Copy Link

@JacquesLucke No, the code that re-inserts local modifiers into a liboverride when the liboverride is re-created from the linked data (rna_Object_modifiers_override_apply), calls ED_object_modifier_add, which will ensure that the inserted modifier has a unique name.

It does mean that names of local modifiers of a liboverride will be modified, in case there is a collision with a modifier from the linked data.

@JacquesLucke No, the code that re-inserts local modifiers into a liboverride when the liboverride is re-created from the linked data (`rna_Object_modifiers_override_apply`), calls `ED_object_modifier_add`, which will ensure that the inserted modifier has a unique name. It does mean that names of local modifiers of a liboverride will be modified, in case there is a collision with a modifier from the linked data.
Jacques Lucke added 2 commits 2024-01-24 10:05:32 +01:00
Jacques Lucke commented 2024-01-24 10:15:40 +01:00
Author
Member
Copy Link

The more I think about it, the more I come to the conclusion that we should rely on randomness with the (very unlikely) occasional collision that is automatically fixed by override-code.

The "ranges of persistent_uid" sound generally like a good idea but it feels like it's more likely to run into issues with it in practice. One case that you haven't considered yet (I think) is when a previously linked data block with extra modifiers is made local and then linked again. In this situation there one can still run into collisions even if one does not have 128 levels of liboverrides at once. Still somewhat unlikely of course, but who knows what people will do with scripts..

Previously, "real randomness" was replaced with hashing in this patch to be more deterministic. This change would have to reverted. However, I think we could still have the determinism for tests if it causes problems. We could add pass a flag to Blender that makes it use a deterministic source of randomness when using RandomNumberGenerator::from_random_seed. This might also be useful for other e.g. operator tests which use randomness.

The more I think about it, the more I come to the conclusion that we should rely on randomness with the (very unlikely) occasional collision that is automatically fixed by override-code. The "ranges of `persistent_uid`" sound generally like a good idea but it feels like it's more likely to run into issues with it in practice. One case that you haven't considered yet (I think) is when a previously linked data block with extra modifiers is made local and then linked again. In this situation there one can still run into collisions even if one does not have 128 levels of liboverrides at once. Still somewhat unlikely of course, but who knows what people will do with scripts.. Previously, "real randomness" was replaced with hashing in this patch to be more deterministic. This change would have to reverted. However, I think we could still have the determinism for tests if it causes problems. We could add pass a flag to Blender that makes it use a deterministic source of randomness when using `RandomNumberGenerator::from_random_seed`. This might also be useful for other e.g. operator tests which use randomness.
Bastien Montagne commented 2024-01-24 10:58:03 +01:00
Owner
Copy Link

The more I think about it, the more I come to the conclusion that we should rely on randomness with the (very unlikely) occasional collision that is automatically fixed by override-code.

The "ranges of persistent_uid" sound generally like a good idea but it feels like it's more likely to run into issues with it in practice. One case that you haven't considered yet (I think) is when a previously linked data block with extra modifiers is made local and then linked again. In this situation there one can still run into collisions even if one does not have 128 levels of liboverrides at once. Still somewhat unlikely of course, but who knows what people will do with scripts..

Indeed. Am also not so found of this idea, as it adds yet another 'unrelated burden' on an already overly complex liboverride code.

Previously, "real randomness" was replaced with hashing in this patch to be more deterministic. This change would have to reverted. However, I think we could still have the determinism for tests if it causes problems. We could add pass a flag to Blender that makes it use a deterministic source of randomness when using RandomNumberGenerator::from_random_seed. This might also be useful for other e.g. operator tests which use randomness.

Can't we still keep the current hash-based randomness, but as suggested in my previous comment, generate the seed hash using more contextual data (specifically, the library absolute path of the liboverride linked reference object?) This should ensure that modifiers with the same name, but created 'locally' in a liboverride, will (almost) never get the same base hash as the modifiers created in the library file on the 'really local' object?

> The more I think about it, the more I come to the conclusion that we should rely on randomness with the (very unlikely) occasional collision that is automatically fixed by override-code. > > The "ranges of `persistent_uid`" sound generally like a good idea but it feels like it's more likely to run into issues with it in practice. One case that you haven't considered yet (I think) is when a previously linked data block with extra modifiers is made local and then linked again. In this situation there one can still run into collisions even if one does not have 128 levels of liboverrides at once. Still somewhat unlikely of course, but who knows what people will do with scripts.. Indeed. Am also not so found of this idea, as it adds yet another 'unrelated burden' on an already overly complex liboverride code. > Previously, "real randomness" was replaced with hashing in this patch to be more deterministic. This change would have to reverted. However, I think we could still have the determinism for tests if it causes problems. We could add pass a flag to Blender that makes it use a deterministic source of randomness when using `RandomNumberGenerator::from_random_seed`. This might also be useful for other e.g. operator tests which use randomness. Can't we still keep the current hash-based randomness, but as suggested in my previous comment, generate the seed hash using more contextual data (specifically, the library absolute path of the liboverride linked reference object?) This should ensure that modifiers with the same name, but created 'locally' in a liboverride, will (almost) never get the same base hash as the modifiers created in the library file on the 'really local' object?
Bastien Montagne commented 2024-01-24 10:59:28 +01:00
Owner
Copy Link

I guess that for the unlikely collision fixing in liboverrides, we'd use the same logic as for modifiers' names? I.e. re-generate a persistent UID for the colliding local modifier?

I guess that for the unlikely collision fixing in liboverrides, we'd use the same logic as for modifiers' names? I.e. re-generate a persistent UID for the colliding local modifier?
Jacques Lucke commented 2024-01-24 11:36:02 +01:00
Author
Member
Copy Link

Can't we still keep the current hash-based randomness, but as suggested in my previous comment, generate the seed hash using more contextual data (specifically, the library absolute path of the liboverride linked reference object?) This should ensure that modifiers with the same name, but created 'locally' in a liboverride, will (almost) never get the same base hash as the modifiers created in the library file on the 'really local' object?

We could always add more contextual data to the hashing to make collisions less likely. To me it just feels like it would be more complex than the alternative of just using random values and not making any guarantees for how the identifier is derived. I understand the debugging/testing argument against randomness, but as mentioned I think that this can be solved in a way that also benefits other parts of Blender.

I guess that for the unlikely collision fixing in liboverrides, we'd use the same logic as for modifiers' names? I.e. re-generate a persistent UID for the colliding local modifier?

Yes, that's what I thought too.

> Can't we still keep the current hash-based randomness, but as suggested in my previous comment, generate the seed hash using more contextual data (specifically, the library absolute path of the liboverride linked reference object?) This should ensure that modifiers with the same name, but created 'locally' in a liboverride, will (almost) never get the same base hash as the modifiers created in the library file on the 'really local' object? We could always add more contextual data to the hashing to make collisions less likely. To me it just feels like it would be more complex than the alternative of just using random values and not making any guarantees for how the identifier is derived. I understand the debugging/testing argument against randomness, but as mentioned I think that this can be solved in a way that also benefits other parts of Blender. > I guess that for the unlikely collision fixing in liboverrides, we'd use the same logic as for modifiers' names? I.e. re-generate a persistent UID for the colliding local modifier? Yes, that's what I thought too.
Jacques Lucke added 2 commits 2024-01-26 10:18:26 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
18a9c5f7dc take library file path into account when generating uid
Jacques Lucke commented 2024-01-26 10:20:03 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Jacques Lucke added 1 commit 2024-01-29 09:34:11 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
2616cd548d Merge branch 'main' into modifier-identifier
Jacques Lucke commented 2024-01-29 09:34:34 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Jacques Lucke added 1 commit 2024-01-29 10:30:01 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
ddb214a061 fix compilation
Jacques Lucke commented 2024-01-29 10:30:12 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Jacques Lucke requested review from Bastien Montagne 2024-01-29 10:30:48 +01:00
Bastien Montagne requested changes 2024-02-06 16:50:38 +01:00
Bastien Montagne left a comment
Owner
Copy Link

So you decided to go the 'more contextual hash' way in the end? ;)

I think this patch is still missing the conflict solving for liboverrides 'local' modifiers though?

So you decided to go the 'more contextual hash' way in the end? ;) I think this patch is still missing the conflict solving for liboverrides 'local' modifiers though?
source/blender/blenkernel/intern/modifier.cc Outdated
@ -1023,0 +1035,4 @@
void BKE_modifiers_persistent_uid_init(const Object &object, ModifierData &md)
{
uint64_t hash = blender::get_default_hash(blender::StringRef(md.name));
if (object.id.lib) {
Bastien Montagne commented 2024-02-06 16:47:31 +01:00
Owner
Copy Link

Direct check on id.lib pointer should never be needed, please use ID_IS_LINKED macro.

Taking the object' lib is probably also necessary (to cover recursive liboverride case, which should be the only case where we can end up creating modifiers in linked data). But this is still missing the main 'basic' case liboverride case. Think you also need something like:

if (ID_IS_OVERRIDE_LIBRARY_REAL(object) {
  BLI_assert(ID_IS_LINKED(object.id.override_library.reference);
  hash = blender::get_default_hash(hash, blender::StringRef(object.id.override_library.reference.lib->filepath_abs));
}
Direct check on `id.lib` pointer should never be needed, please use `ID_IS_LINKED` macro. ------- Taking the object' lib is probably also necessary (to cover recursive liboverride case, which should be the only case where we can end up creating modifiers in linked data). But this is still missing the main 'basic' case liboverride case. Think you also need something like: ``` if (ID_IS_OVERRIDE_LIBRARY_REAL(object) { BLI_assert(ID_IS_LINKED(object.id.override_library.reference); hash = blender::get_default_hash(hash, blender::StringRef(object.id.override_library.reference.lib->filepath_abs)); } ```
JacquesLucke marked this conversation as resolved
Jacques Lucke commented 2024-02-06 16:56:53 +01:00
Author
Member
Copy Link

I think this patch is still missing the conflict solving for liboverrides 'local' modifiers though?

rna_Object_modifiers_override_apply calls ED_object_modifier_add which ensures a unique name as well as unique persistent_uid. That's what you meant, right?

So you decided to go the 'more contextual hash' way in the end? ;)

Yeah, for now anyway. Makes more people happy and it's fine with me.

> I think this patch is still missing the conflict solving for liboverrides 'local' modifiers though? `rna_Object_modifiers_override_apply` calls `ED_object_modifier_add` which ensures a unique name as well as unique `persistent_uid`. That's what you meant, right? > So you decided to go the 'more contextual hash' way in the end? ;) Yeah, for now anyway. Makes more people happy and it's fine with me.
Bastien Montagne commented 2024-02-06 17:04:23 +01:00
Owner
Copy Link

I think this patch is still missing the conflict solving for liboverrides 'local' modifiers though?

rna_Object_modifiers_override_apply calls ED_object_modifier_add which ensures a unique name as well as unique persistent_uid. That's what you meant, right?

Aaaah my bad, yes you are right. Then besides notes in comment this PR LGTM.

> > I think this patch is still missing the conflict solving for liboverrides 'local' modifiers though? > > `rna_Object_modifiers_override_apply` calls `ED_object_modifier_add` which ensures a unique name as well as unique `persistent_uid`. That's what you meant, right? > Aaaah my bad, yes you are right. Then besides notes in comment this PR LGTM.
Jacques Lucke added 2 commits 2024-02-06 17:04:31 +01:00
Bastien Montagne approved these changes 2024-02-06 17:05:31 +01:00
Jacques Lucke merged commit 1497005054 into main 2024-02-06 17:10:48 +01:00
Jacques Lucke deleted branch modifier-identifier 2024-02-06 17:10:51 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Brecht Van Lommel
Hans Goudey
Sergey Sharybin
Bastien Montagne
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset Browser

  
Interest
Asset Browser Project Overview

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
This issue affects/is about backward or forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
EEVEE & Viewport

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline, Assets & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
Legacy
Blender 2.8 Project

  
Legacy
Milestone 1: Basic, Local Asset Browser

  
Legacy
OpenGL Error

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
Module
EEVEE & Viewport

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline, Assets & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Priority
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#117347
No description provided.
Delete Branch "JacquesLucke/blender:modifier-identifier"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?