LibOverride fixes for Blender 3.6.2 LTS #109704

Bastien Montagne · 2023-07-04T17:16:25+02:00

Bastien Montagne commented

2023-07-04 17:16:25 +02:00

This PR contains several fixes for liboverrides when production files get very messy.

Parenting-related Object properties (the parent inverse matrix, parent type, parent sub-target) will now be reset to the linked reference data when parent pointer itself is modified in library file and updated during resync process.
Cleanup-code pruning unused liboverride operations during RNA diffing would not always work properly, leading to accumulated invalid operations that could, under certain circumstances, end up severely breaking especially collections/objects hierarchies of liboverrides.
Resync Enforce tool would not always work properly with collections of IDs (affecting especially Collections/Objects relationships).
Handling of lookup of items in RNA collections has been improved, to alleviate issues when there are name collisions between items.

This PR contains several fixes for liboverrides when production files get very messy. * Parenting-related Object properties (the parent inverse matrix, parent type, parent sub-target) will now be reset to the linked reference data when parent pointer itself is modified in library file and updated during resync process. * Cleanup-code pruning unused liboverride operations during RNA diffing would not always work properly, leading to accumulated invalid operations that could, under certain circumstances, end up severely breaking especially collections/objects hierarchies of liboverrides. * Resync Enforce tool would not always work properly with collections of IDs (affecting especially Collections/Objects relationships). * Handling of lookup of items in RNA collections has been improved, to alleviate issues when there are name collisions between items.

Bastien Montagne commented

2023-07-04 17:17:19 +02:00

@blender-bot package

Blender Bot commented

2023-07-04 17:17:22 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR109704) when ready.

Bastien Montagne force-pushed tmp-liboverride-resync-parenting from 6ed0a9fc62 to beb4a67cd6

2023-07-04 18:02:19 +02:00

Compare

Bastien Montagne commented

2023-07-04 18:02:50 +02:00

@blender-bot package

Blender Bot commented

2023-07-04 18:02:53 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR109704) when ready.

Bastien Montagne force-pushed tmp-liboverride-resync-parenting from beb4a67cd6 to 226300c48f

2023-07-04 18:33:54 +02:00

Compare

Bastien Montagne commented

2023-07-04 18:34:03 +02:00

@blender-bot package

Blender Bot commented

2023-07-04 18:34:06 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR109704) when ready.

Bastien Montagne commented

2023-07-05 09:35:22 +02:00

@Mets you have the build ;) https://builder.blender.org/download/patch/PR109704

Demeter Dzadik commented

2023-07-06 15:44:01 +02:00

Context:

Original issue we encountered was that some objects of an asset in a shot file had the default matrix as their parent inverse matrix, even though in the source file, it was some specific value, set by the parenting operator as usual.

We fixed it on our end by simply deleting the linked and overridden asset, and linking and overriding it back in, and restoring relationships and whatever else we needed. (I actually wrote an operator to do all this now, only for emergencies.)

Testing:

First I tried reproducing the original issue from scratch without the patch, and failed, actually.
I tried changing parenting types from object to bone and back, both using the operators and the properties in the UI, I tried duplicating objects with various parenting types and changing them as well, but everything propagated successfully to the shot file. Again, even without the patch.
So, it's still a mystery to me how we managed to get the file into that state.
Then I reverted our shot file from version control back to its broken state, from before we re-linked the asset. With the patch build, I tried save&reload to see if the objects would regain their parent inverse matrix, but they didn't. Also tried Resync Enforce, and then save and reload, but still nothing.

So I would say the patch doesn't seem to fix the broken file, but without reproduction steps I don't know what else we can do either other than keep an eye out until we find those repro steps.

Context: Original issue we encountered was that some objects of an asset in a shot file had the default matrix as their parent inverse matrix, even though in the source file, it was some specific value, set by the parenting operator as usual. We fixed it on our end by simply deleting the linked and overridden asset, and linking and overriding it back in, and restoring relationships and whatever else we needed. (I actually wrote an operator to do all this now, only for emergencies.) Testing: 1. First I tried reproducing the original issue from scratch without the patch, and failed, actually. I tried changing parenting types from object to bone and back, both using the operators and the properties in the UI, I tried duplicating objects with various parenting types and changing them as well, but everything propagated successfully to the shot file. Again, even without the patch. So, it's still a mystery to me how we managed to get the file into that state. 2. Then I reverted our shot file from version control back to its broken state, from before we re-linked the asset. With the patch build, I tried save&reload to see if the objects would regain their parent inverse matrix, but they didn't. Also tried Resync Enforce, and then save and reload, but still nothing. So I would say the patch doesn't seem to fix the broken file, but without reproduction steps I don't know what else we can do either other than keep an eye out until we find those repro steps.

Bastien Montagne commented

2023-07-06 16:17:29 +02:00

This is not expected to fix currently broken files (there is just no way we can blindly assume all overrides in invert parent matrix properties are invalid). It will however (hopefully) prevent this issue to happen again, since it will reset this property (and other related to parenting) to linked data values when the parent ID itself (the child object) is resynced, and the parent ID liboverride operation is a system override one (i.e. it matches the parent ID in the linked data). which will happen, among other cases, when said parent ID is changed in the library file.

So to test you need to reparent something in the library file, then open the anim file, and check that the re-parented object got back to its expected place.

Commit message/PR description is lacking about that info, sorry, will update them asap.

This is not expected to fix currently broken files (there is just no way we can blindly assume all overrides in invert parent matrix properties are invalid). It will however (hopefully) prevent this issue to happen again, since it will reset this property (and other related to parenting) to linked data values when the parent ID itself (the child object) is resynced, and the parent ID liboverride operation is a system override one (i.e. it matches the parent ID in the linked data). which will happen, among other cases, when said parent ID is changed in the library file. So to test you need to reparent something in the library file, then open the anim file, and check that the re-parented object got back to its expected place. Commit message/PR description is lacking about that info, sorry, will update them asap.

Demeter Dzadik commented

2023-07-06 16:22:15 +02:00

Right, but the weird thing is, I was trying to make parenting changes in the library file.
In 3.1 RC without the patch, and it just worked!

See this part of my comment:

I tried changing parenting types from object to bone and back, both using the operators and the properties in the UI, I tried duplicating objects with various parenting types and changing them as well, but everything propagated successfully to the shot file.

I expected this, without the patch:

Open library file, aka asset file, aka rocket_interior.blend
Make changes to the object parenting, including parenting types
Open shot file where the asset is linked and overridden
Objects should be in the wrong positions because their parent inverse matrix fails to update

But the last step actually did not happen. So either I'm missing something, or the repro steps are more specific than we thought.

Right, but the weird thing is, I _was_ trying to make parenting changes in the library file. In 3.1 RC without the patch, and it just worked! See this part of my comment: > I tried changing parenting types from object to bone and back, both using the operators and the properties in the UI, I tried duplicating objects with various parenting types and changing them as well, but everything propagated successfully to the shot file. I expected this, without the patch: - Open library file, aka asset file, aka `rocket_interior.blend` - Make changes to the object parenting, including parenting types - Open shot file where the asset is linked and overridden - Objects should be in the wrong positions because their parent inverse matrix fails to update But the last step actually did not happen. So either I'm missing something, or the repro steps are more specific than we thought.

Bastien Montagne commented

2023-07-06 16:41:38 +02:00

How the parent inverse matrix ended up being overridden in the first place is a big mystery. I can imagine two scenarii:

Bad conjunctions of planets (files were saved with a specific version of Blender 3.6 alpha (aka main at the time) that ended up somehow messing up things, creating unmatching invert matrices and generating liboverride operation for this property.
Some artists using some very exotic feature or add-on that ended up breaking the inverse parent matrix. Maybe something as "dummy" as changing the parenting type in the UI, saving, re-opening, and changing it back to expected value ?

How the parent inverse matrix ended up being overridden in the first place _is_ a big mystery. I can imagine two scenarii: - Bad conjunctions of planets (files were saved with a specific version of Blender 3.6 alpha (aka main at the time) that ended up somehow messing up things, creating unmatching invert matrices and generating liboverride operation for this property. - Some artists using some very exotic feature or add-on that ended up breaking the inverse parent matrix. Maybe something as "dummy" as changing the parenting type in the UI, saving, re-opening, and changing it back to expected value ?

Demeter Dzadik commented

2023-07-06 16:50:07 +02:00

Hmm, I'd probably go with the first option.

Still, that means I can't really test the patch in a meaningful way, but of course if you think it's an improvement in any case, and since at least I did test that it doesn't make anything worse, it's good to go?

Hmm, I'd probably go with the first option. Still, that means I can't really test the patch in a meaningful way, but of course if you think it's an improvement in any case, and since at least I did test that it doesn't make anything worse, it's good to go?

Bastien Montagne commented

2023-07-06 16:56:44 +02:00

Will update the PRs with fixes for borkend collections cases (the 050_0070 case) as well, hopefully later today.

You should be able to test it though by reverting your SVN to before you fixed the files?

Will update the PRs with fixes for borkend collections cases (the 050_0070 case) as well, hopefully later today. You should be able to test it though by reverting your SVN to before you fixed the files?

Demeter Dzadik commented

2023-07-06 17:13:23 +02:00

Right, talked in chat, this is what I should've tested, and have tested now:

Revert shot file to broken state (objects flying away due to bad parent inverse matrix)
Change parenting of those objects in the library file
Reload shot file
Without the patch, the objects are still flying away, but with the patch, the objects are in the correct positions! ✔️

Right, talked in chat, this is what I should've tested, and have tested now: - Revert shot file to broken state (objects flying away due to bad parent inverse matrix) - Change parenting of those objects in the library file - Reload shot file - Without the patch, the objects are still flying away, but with the patch, the objects are in the correct positions! ✔️

Bastien Montagne force-pushed tmp-liboverride-resync-parenting from 226300c48f to fd7bf13a98

2023-07-08 16:34:08 +02:00

Compare

Bastien Montagne commented

2023-07-08 16:34:32 +02:00

@blender-bot package

Blender Bot commented

2023-07-08 16:34:35 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR109704) when ready.

Bastien Montagne commented

2023-07-08 16:39:44 +02:00

@Mets with this you should now be able to fix even the insanely broken cases like 050_0070.anim, with an extra save & reload first to get rid of unused liboverride operations, and then the usual manual cleanup of garbage data + resync (enforce).

@Mets with this you should now be able to fix even the insanely broken cases like `050_0070.anim`, with an extra save & reload first to get rid of unused liboverride operations, and then the usual manual cleanup of garbage data + resync (enforce).

Demeter Dzadik commented

2023-07-10 15:48:55 +02:00

Yep, tested the latest build, seems to repair everything in shot 050_0070 with Resync Enforce.
Only thing is that the OVERRIDE_HIDDEN collection has to be removed manually, but I think that's a good thing.

This is not expected to fix currently broken files

It seems to not only fix collection assignments, but also the inverse matrices. Sounds like a good thing, but I hope it's also expected?

Also, I recall you mentioned IRL something about datablock names. Is that something in this patch to be tested, or something for the future?

Thanks a lot for your hard work, Bastien! Good to go as far as I'm concerned, depending on those questions.

Yep, tested the latest build, seems to repair everything in shot 050_0070 with Resync Enforce. Only thing is that the OVERRIDE_HIDDEN collection has to be removed manually, but I think that's a good thing. > This is not expected to fix currently broken files It seems to not only fix collection assignments, but also the inverse matrices. Sounds like a good thing, but I hope it's also expected? Also, I recall you mentioned IRL something about datablock names. Is that something in this patch to be tested, or something for the future? Thanks a lot for your hard work, Bastien! Good to go as far as I'm concerned, depending on those questions.

Demeter Dzadik commented

2023-07-10 16:07:29 +02:00

Oh, one potentially interesting note though:
A file saved with the patch seems to crash on load with 3.6.1 RC.
Not sure if that's meant to be supported though, and to what extent.

Oh, one potentially interesting note though: A file saved with the patch seems to crash on load with 3.6.1 RC. Not sure if that's meant to be supported though, and to what extent.

Bastien Montagne commented

2023-07-10 16:11:18 +02:00

Parenting issues would indeed be fixed when rebuilding/fixing collections and objects relationships yes.

Datablock naming change is the following: When an override is created from a linked reference ID, and it cannot get the exact same name as its reference (usually because there is already a local ID with that name, be it an override or not), it will be named such that it does not collide with any existing ID, local or linked.

The goal here is to avoid getting an override ID named the same way as another ID in the same linked asset. E.g. huge issue in 050_0070 is that you ended up having, under the override collection door, object like door.004 which would be an override of linked object door.001, while override object door.005 was an override of linked object door.004.

This could happen due to a mixture of bad usage (the extra, completely hidden, override of the rocket set), complex high volatility of hierarchy in the linked set asset, and results from automatic resync on file opening trying its best to cope with the whole mess.

While the change on naming is not 100% bullet proof, together with the other fixes and improvements in that PR it should make such issues due to name collisions extremely rare. Although I would still heavily recommend forbidding anything like door, door.001, door.004 etc. collections of objects names in a production asset file.

Parenting issues would indeed be fixed when rebuilding/fixing collections and objects relationships yes. Datablock naming change is the following: When an override is created from a linked reference ID, and it cannot get the exact same name as its reference (usually because there is already a local ID with that name, be it an override or not), it will be named such that it does not collide with any existing ID, local or linked. The goal here is to avoid getting an override ID named the same way as another ID in the same linked asset. E.g. huge issue in `050_0070` is that you ended up having, under the override collection `door`, object like `door.004` which would be an override of linked object `door.001`, while override object `door.005` was an override of linked object `door.004`. This could happen due to a mixture of bad usage (the extra, completely hidden, override of the rocket set), complex high volatility of hierarchy in the linked set asset, and results from automatic resync on file opening trying its best to cope with the whole mess. While the change on naming is not 100% bullet proof, together with the other fixes and improvements in that PR it should make such issues due to name collisions extremely rare. Although I would still heavily recommend forbidding anything like `door`, `door.001`, `door.004` etc. collections of objects names in a production asset file.

Bastien Montagne commented

2023-07-10 16:11:59 +02:00

Oh, one potentially interesting note though:
A file saved with the patch seems to crash on load with 3.6.1 RC.
Not sure if that's meant to be supported though, and to what extent.

Can you give me one of these?

> Oh, one potentially interesting note though: > A file saved with the patch seems to crash on load with 3.6.1 RC. > Not sure if that's meant to be supported though, and to what extent. Can you give me one of these?

Demeter Dzadik commented

2023-07-10 16:46:30 +02:00

Due to file size, I saved it in the Pet Projects SVN under pets/bugs/050_0070_.....109704.blend.

So to clarify my steps:

Reverted to the broken state of the file back on July 4.
Open the file with the build with the patch, run Resync Enforce on SE-rocket_interior, save file
Try to open file with 3.6.1 RC build
Crash on load.

Another thing I only now noticed (sorry) is that on file save, transforms of overridden objects seem to get reset to default. But since everything is keyframed, moving the timeline snaps everything back in place. Maybe that's why I didn't notice it earlier.

Due to file size, I saved it in the Pet Projects SVN under `pets/bugs/050_0070_.....109704.blend`. So to clarify my steps: - Reverted to the broken state of the file back on July 4. - Open the file with the build with the patch, run Resync Enforce on SE-rocket_interior, save file - Try to open file with 3.6.1 RC build - Crash on load. ---------------------------------------------------- Another thing I only now noticed (sorry) is that on file save, transforms of overridden objects seem to get reset to default. But since everything is keyframed, moving the timeline snaps everything back in place. Maybe that's why I didn't notice it earlier.

Bastien Montagne force-pushed tmp-liboverride-resync-parenting from fd7bf13a98 to 6c0e3a61ec

2023-07-10 17:33:34 +02:00

Compare

Bastien Montagne commented

2023-07-10 17:35:00 +02:00

Due to file size, I saved it in the Pet Projects SVN under pets/bugs/050_0070_.....109704.blend.

So to clarify my steps:

Reverted to the broken state of the file back on July 4.

Open the file with the build with the patch, run Resync Enforce on SE-rocket_interior, save file

Try to open file with 3.6.1 RC build

Crash on load.

Ah yes, this one of the things fixed by this PR (in 1b76b958869b).

Another thing I only now noticed (sorry) is that on file save, transforms of overridden objects seem to get reset to default. But since everything is keyframed, moving the timeline snaps everything back in place. Maybe that's why I didn't notice it earlier.

not sure what you mean here...

> Due to file size, I saved it in the Pet Projects SVN under `pets/bugs/050_0070_.....109704.blend`. > > So to clarify my steps: > - Reverted to the broken state of the file back on July 4. > - Open the file with the build with the patch, run Resync Enforce on SE-rocket_interior, save file > - Try to open file with 3.6.1 RC build > - Crash on load. Ah yes, this one of the things fixed by this PR (in 1b76b958869b). > Another thing I only now noticed (sorry) is that on file save, transforms of overridden objects seem to get reset to default. But since everything is keyframed, moving the timeline snaps everything back in place. Maybe that's why I didn't notice it earlier. not sure what you mean here...

Bastien Montagne commented

2023-07-10 18:00:14 +02:00

Another thing I only now noticed (sorry) is that on file save, transforms of overridden objects seem to get reset to default. But since everything is keyframed, moving the timeline snaps everything back in place. Maybe that's why I didn't notice it earlier.

Should be 'fixed' with last commit.

> Another thing I only now noticed (sorry) is that on file save, transforms of overridden objects seem to get reset to default. But since everything is keyframed, moving the timeline snaps everything back in place. Maybe that's why I didn't notice it earlier. Should be 'fixed' with last commit.

Bastien Montagne commented

2023-07-11 12:37:38 +02:00

@blender-bot package

Blender Bot commented

2023-07-11 12:37:42 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR109704) when ready.

Bastien Montagne force-pushed tmp-liboverride-resync-parenting from 0f54f66254 to 09fb8e2e9f

2023-07-11 16:12:33 +02:00

Compare

Bastien Montagne changed title from ~~tmp-liboverride-resync-parenting~~ to LibOverride fixes for Blender 3.6.2 LTS

2023-07-11 16:13:39 +02:00

Bastien Montagne referenced this pull request

2023-07-11 16:40:03 +02:00

Blender LTS: Maintenance Task 3.6 #109399

Bastien Montagne force-pushed tmp-liboverride-resync-parenting from 09fb8e2e9f to ec376fa6ce

2023-07-11 17:17:38 +02:00

Compare

Bastien Montagne force-pushed tmp-liboverride-resync-parenting from ec376fa6ce to 439ad29453

2023-07-12 17:11:30 +02:00

Compare

Bastien Montagne force-pushed tmp-liboverride-resync-parenting from 439ad29453 to 585f467efe

2023-07-12 17:23:55 +02:00

Compare

Bastien Montagne closed this pull request

2023-07-31 17:58:21 +02:00

Bastien Montagne reopened this pull request

2023-08-02 12:28:02 +02:00

Bastien Montagne force-pushed tmp-liboverride-resync-parenting from 014a9aef0d to 1d52dbf81d

2023-08-06 23:01:01 +02:00

Compare

Bastien Montagne commented

2023-08-06 23:01:14 +02:00

@blender-bot build

Bastien Montagne added 1 commit 2023-08-06 23:03:52 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

8a45453a9b Fix (unreported) missing 'need resync' detection in overrides of overrides.

In case the reference data of a liboverride is aslo a liboverride, and
is already tagged for resync, the override of the override also needs to
be resynced.

Note that linked overrides are guaranteed to be processed by apply code
(and hence get checked for needed resync) before the liboverrides using
them.

Bastien Montagne commented

2023-08-06 23:04:14 +02:00

@blender-bot build

Bastien Montagne added 1 commit 2023-08-06 23:13:43 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

526dc2b58f LibOverride: Extend unittest to cover more advanced/complex cases.

These tests now have basic coverage of:
* Linking data and creating liboverride hierarchy from it.
* Linking that liboverride again.
* Creating liboverride of liboverride.
* Modifying data hierarchy in the library, and testing (recursive)
  resync of it.
* ID name collision handling (between modified linked data and
  existing overrides).

Bastien Montagne commented

2023-08-06 23:14:22 +02:00

@blender-bot build

Bastien Montagne added 1 commit 2023-08-07 19:13:49 +02:00

684a0b20c3 LibOverride: Remove an override property when it has no operations.

Update #BKE_lib_override_library_id_unused_cleanup to also clear the
property when all its operations have been removed, even if the
property itself was not tagged as unused.

Bastien Montagne commented

2023-08-07 19:14:01 +02:00

@blender-bot build

Bastien Montagne force-pushed tmp-liboverride-resync-parenting from b404dbaccd to 9d978bc2ea

2023-08-08 10:21:01 +02:00

Compare

Bastien Montagne commented

2023-08-08 10:21:17 +02:00

@blender-bot build

Philipp Oeser merged commit f63ca4f7a8 into blender-v3.6-release

2023-08-09 14:34:40 +02:00

Philipp Oeser referenced this issue from a commit