Fix: Various issues with auto smooth versioning #119539

Hans Goudey · 2024-03-15T21:07:22+01:00

Hans Goudey commented

2024-03-15 21:07:22 +01:00

This PR fixes several issues with the versioning that replaces the old
auto smooth flag with a modifier.

One issue is that the flag wasn't cleared in the initial versioning
code. That means some objects have the replacement modifier but their
meshes still have the flag set. The fix for that is to make the
versioning idempotent by trying to find an existing node group before
adding a new one. The versionig is now re-run on all objects to clear
the flags. Flags on all meshes are cleared too, even unused meshes.
That could cause loss of the auto-smooth when the mesh is linked from
a different blend-file, but that situation should be very rare.

Another issue was that the versioning wasn't run when linking objects.
That was simple to solve by adding the versioning where the proxy
versioning already existed for that case.

Finally, arguably the largest issue was that the the newly added node
groups were always added as local data-blocks. When linking, having
library data-blocks point to local data-blocks not in that library is
quite bad and breaks assumptions around Blender. This is solved by
having an auto smooth node group per library.

Resolves #119516, #119455, #119447

This PR fixes several issues with the versioning that replaces the old auto smooth flag with a modifier. One issue is that the flag wasn't cleared in the initial versioning code. That means some objects have the replacement modifier but their meshes still have the flag set. The fix for that is to make the versioning idempotent by trying to find an existing node group before adding a new one. The versionig is now re-run on all objects to clear the flags. Flags on all meshes are cleared too, even unused meshes. That could cause loss of the auto-smooth when the mesh is linked from a different blend-file, but that situation should be very rare. Another issue was that the versioning wasn't run when linking objects. That was simple to solve by adding the versioning where the proxy versioning already existed for that case. Finally, arguably the largest issue was that the the newly added node groups were always added as local data-blocks. When linking, having library data-blocks point to local data-blocks not in that library is quite bad and breaks assumptions around Blender. This is solved by having an auto smooth node group per library. Resolves #119516, #119455, #119447

Hans Goudey added 1 commit 2024-03-15 21:07:29 +01:00

e4c801a1ef Fix: Various issues with auto smooth versioning

Hans Goudey added this to the 4.1 milestone 2024-03-15 21:07:49 +01:00

Hans Goudey added 4 commits 2024-03-18 17:02:41 +01:00

de1a966e24 Fix extra user when appending

b3a7bc7978 Change from comparison to "find auto smooth group" process

e27a272a74 Fix missing versioning for linking

cde84bde87 Properly deal with library

Hans Goudey referenced this pull request

2024-03-18 17:04:42 +01:00

Fix #119516: Auto smooth versioning skipped for multi-user meshes #119471

Hans Goudey added 1 commit 2024-03-18 17:40:31 +01:00

1574030dca Cleanup

Hans Goudey changed title from ~~WIP: Fix: Various issues with auto smooth versioning~~ to Fix: Various issues with auto smooth versioning

2024-03-18 17:54:01 +01:00

Hans Goudey added 2 commits 2024-03-18 18:00:31 +01:00

246222d794 Move code

bd892164ca Correct comments

Hans Goudey requested review from Brecht Van Lommel 2024-03-18 18:05:09 +01:00

Hans Goudey requested review from Jacques Lucke 2024-03-18 18:05:09 +01:00

Jacques Lucke reviewed 2024-03-18 18:19:50 +01:00

source/blender/blenkernel/intern/mesh_legacy_convert.cc Outdated

						
				@ -2244,0 +2256,4 @@

				  return result;

				}

				static bool is_auto_smooth_node_tree(const bNodeTree &group)

Jacques Lucke commented

2024-03-18 18:13:49 +01:00

Would be good to mention the function name that builds the node group we compare against here.

HooglyBoogly marked this conversation as resolved

source/blender/blenkernel/intern/mesh_legacy_convert.cc Outdated

						
				@ -2247,2 +2335,3 @@

				  BKE_modifier_unique_name(&object.modifiers, &md->modifier);

				  md->node_group = get_node_group();

				  md->node_group = get_node_group(object.id.lib);

				  md->node_group->id.us++;

Jacques Lucke commented

2024-03-18 18:16:52 +01:00

Any reason for using direct increment/decrement instead of the corresponding utility methods? (id_us_plus)

Any reason for using direct increment/decrement instead of the corresponding utility methods? (`id_us_plus`)

Hans Goudey commented

2024-03-18 18:22:54 +01:00

Hmm, no, I'll use those

HooglyBoogly marked this conversation as resolved

source/blender/blenkernel/intern/mesh_legacy_convert.cc Outdated

						
				@ -2268,3 +2357,4 @@

				  using namespace blender;

				  using namespace blender::bke;

				  /* Add the node group lazily and share it among all objects in the main database. */

Jacques Lucke commented

2024-03-18 18:11:55 +01:00

This comment seems outdated.

HooglyBoogly marked this conversation as resolved

source/blender/blenkernel/intern/mesh_legacy_convert.cc

						
				@ -2276,0 +2378,4 @@

				    /* Remove the default user. The count is tracked manually when assigning to modifiers. */

				    new_group->id.us--;

				    if (new_group->id.lib != library) {

Jacques Lucke commented

2024-03-18 18:17:57 +01:00

Is this semantically the same as comparing to nullptr here?

Is this semantically the same as comparing to `nullptr` here?

Hans Goudey commented

2024-03-18 18:27:44 +01:00

Semantically I think it's different. This is really asking "do we have to move the data-block to a different library.

Physically I think it's the same (though that assumes a new data-blocks .lib pointer is always nullptr, which I'm not 100% sure about with the curlib stuff. So I'd rather be safe here since this check doesn't hurt.

Semantically I think it's different. This is really asking "do we have to move the data-block to a different library. Physically I think it's the same (though that assumes a new data-blocks `.lib` pointer is always `nullptr`, which I'm not 100% sure about with the `curlib` stuff. So I'd rather be safe here since this check doesn't hurt.

source/blender/blenkernel/intern/mesh_legacy_convert.cc

						
				@ -2276,0 +2382,4 @@

				      /* Move the node group to the requested library so that library data-blocks don't point to

				       * local data-blocks. This requires making sure the name is unique in that library and

				       * changing the name maps to be consistent with the new state. */

				      new_group->id.lib = library;

Jacques Lucke commented

2024-03-18 18:19:32 +01:00

Are there other places which move something into a library after it has been created? Seems ok, but don't know if just setting this one property is enough to make it part of the library.

Hans Goudey commented

2024-03-18 18:25:58 +01:00

As far as I can tell (reading other code that does ->lib = ), yeah, this looks like enough. Bastien also said this:

trivial to fix now in main, using the new BKE_id_new_in_lib functions. In 4.1, I can only think of creating a local nodetree, assigning the library to it, and re-running the uniquename/sorting code (BKE_id_new_name_validate) on it to ensure it ends up where it should and does have a unique name in the library namespace.

As far as I can tell (reading other code that does `->lib = `), yeah, this looks like enough. Bastien also said this: >trivial to fix now in main, using the new BKE_id_new_in_lib functions. In 4.1, I can only think of creating a local nodetree, assigning the library to it, and re-running the uniquename/sorting code (BKE_id_new_name_validate) on it to ensure it ends up where it should and does have a unique name in the library namespace.

Jacques Lucke commented

2024-03-18 18:47:29 +01:00

Out of the flags I checked, LIB_TAG_EXTERN may need to be set, but hard to say for sure.

Out of the flags I checked, `LIB_TAG_EXTERN` may need to be set, but hard to say for sure.

Brecht Van Lommel commented

2024-03-18 19:24:49 +01:00

I think LIB_TAG_INDIRECT should be set, which means linked indirectly through another linked datablock. LIB_TAG_EXTERN means linked directly.

I think `LIB_TAG_INDIRECT` should be set, which means linked indirectly through another linked datablock. `LIB_TAG_EXTERN` means linked directly.

Brecht Van Lommel commented

2024-03-18 19:26:52 +01:00

One thing I would suggest to test is to link an object datablock like this, and then make it local. See if it gets correctly converted.

Hans Goudey commented

2024-03-18 19:48:20 +01:00

When I make a linked object local, the node group remains linked. The object's mesh data does too though, so I wouldn't expect the node group to become local.

Semantically LIB_TAG_INDIRECT makes sense though, so I'll add the tag (it didn't change the object make local behavior anyway).

I also checked "Make Local" on the node group itself, that does work.

When I make a linked object local, the node group remains linked. The object's mesh data does too though, so I wouldn't expect the node group to become local. Semantically `LIB_TAG_INDIRECT` makes sense though, so I'll add the tag (it didn't change the object make local behavior anyway). I also checked "Make Local" on the node group itself, that does work.

Hans Goudey added 4 commits 2024-03-18 18:29:39 +01:00

590c027f3e Merge branch 'blender-v4.1-release' into fix-auto-smooth-versioning

20cd04263f Add comment to `is_auto_smooth_node_tree`

5d550b0a95 Use utilities for ID user counts

bb218da582 Fix outdated comment

Jacques Lucke approved these changes 2024-03-18 18:44:38 +01:00

Jacques Lucke left a comment

Also fixes the bug I was investigating this morning in Simons file.

source/blender/blenkernel/intern/mesh_legacy_convert.cc Outdated

						
				@ -37,2 +37,4 @@

				#include "BKE_idprop.hh"

				#include "BKE_lib_id.hh"

				#include "BKE_main.hh"

				#include "BKE_main_namemap.hh"

Jacques Lucke commented

2024-03-18 18:42:20 +01:00

Might need #include "BKE_node_runtime.hh" for .all_nodes().

Might need `#include "BKE_node_runtime.hh"` for `.all_nodes()`.

Hans Goudey commented

2024-03-18 19:04:05 +01:00

Already there :)

HooglyBoogly marked this conversation as resolved

Hans Goudey added 1 commit 2024-03-18 19:04:46 +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

b3b931e726 Remove unnecessary include

Hans Goudey commented

2024-03-18 19:04:52 +01:00

@blender-bot build

Hans Goudey added 2 commits 2024-03-18 19:48:44 +01:00

ecda87fd48 Merge branch 'blender-v4.1-release' into fix-auto-smooth-versioning

c88838a303 Add `LIB_TAG_INDIRECT` tag

Brecht Van Lommel approved these changes 2024-03-18 20:00:08 +01:00

Brecht Van Lommel left a comment

Code looks good, seems to work fine in testing.

Hans Goudey merged commit 5bfe6ad8f8 into blender-v4.1-release

2024-03-18 20:15:45 +01:00

Hans Goudey referenced this issue from a commit

2024-03-18 20:15:46 +01:00

Fix: Various issues with auto smooth versioning

Hans Goudey deleted branch fix-auto-smooth-versioning

2024-03-18 20:15:52 +01:00

Sybren A. Stüvel commented

2024-03-21 14:02:50 +01:00

This unfortunately causes a crash for me on the Amy Restaurant demo file:

ERROR (bke.main_namemap): /home/sybren/workspace/blender-git/blender/source/blender/blenkernel/intern/main_namemap.cc:585 main_namemap_validate_and_fix: ID name 'NTAuto Smooth.001' (from library '<None>') is listed in the namemap, but does not exists in current Main
ERROR (bke.main_namemap): /home/sybren/workspace/blender-git/blender/source/blender/blenkernel/intern/main_namemap.cc:585 main_namemap_validate_and_fix: ID name 'NTAuto Smooth.002' (from library '<None>') is listed in the namemap, but does not exists in current Main
blenderdebug() [0x9610a9b]
blenderdebug() [0xc905aba]
blenderdebug() [0xc117547]
blenderdebug() [0x96c6045]
blenderdebug() [0x96bc85a]
blenderdebug() [0x96bc71a]
blenderdebug() [0xccbb8b3]
blenderdebug() [0x96965cd]
blenderdebug() [0x96963ba]
blenderdebug() [0xc93700e]
blenderdebug() [0x9695d54]
/lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7f7c1227ed90]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7f7c1227ee40]
blenderdebug() [0x95d4f65]
BLI_assert failed: /home/sybren/workspace/blender-git/blender/source/blender/blenkernel/intern/blendfile.cc:942, setup_app_data(), at 'BKE_main_namemap_validate(bmain)'

This unfortunately causes a crash for me on the [Amy Restaurant demo file](https://studio.blender.org/characters/5f1ed640e9115ed35ea4b3fb/showcase/1/): ``` ERROR (bke.main_namemap): /home/sybren/workspace/blender-git/blender/source/blender/blenkernel/intern/main_namemap.cc:585 main_namemap_validate_and_fix: ID name 'NTAuto Smooth.001' (from library '<None>') is listed in the namemap, but does not exists in current Main ERROR (bke.main_namemap): /home/sybren/workspace/blender-git/blender/source/blender/blenkernel/intern/main_namemap.cc:585 main_namemap_validate_and_fix: ID name 'NTAuto Smooth.002' (from library '<None>') is listed in the namemap, but does not exists in current Main blenderdebug() [0x9610a9b] blenderdebug() [0xc905aba] blenderdebug() [0xc117547] blenderdebug() [0x96c6045] blenderdebug() [0x96bc85a] blenderdebug() [0x96bc71a] blenderdebug() [0xccbb8b3] blenderdebug() [0x96965cd] blenderdebug() [0x96963ba] blenderdebug() [0xc93700e] blenderdebug() [0x9695d54] /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7f7c1227ed90] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7f7c1227ee40] blenderdebug() [0x95d4f65] BLI_assert failed: /home/sybren/workspace/blender-git/blender/source/blender/blenkernel/intern/blendfile.cc:942, setup_app_data(), at 'BKE_main_namemap_validate(bmain)' ```

Sybren A. Stüvel referenced this pull request

2024-03-21 15:22:03 +01:00

Fix: ID namemap assert in auto smooth versioning code #119747

Sign in to join this conversation.

No reviewers