From 01ddb320dd99be718dcefe3662129208894218bf Mon Sep 17 00:00:00 2001 From: Jesse Yurkovich Date: Fri, 21 Mar 2025 21:29:08 +0100 Subject: [PATCH] Fix #124263: Generate unique names during Alembic and USD export While object names in Blender are already unique, the names themselves may be "unsafe" for use in the various file formats. During processing we make the names "safe". However, we did not guarantee that these new safe names were themselves unique wrt each other. Consider object names "Test 1" and "Test-1" which both become "Test_1" after being made safe. These will collide during export; only 1 object would be exported and it's undefined which object's data would "win". To rectify this we add another name map to the hierarchy iterator which is then used to handle collisions as they happen. The map is per- hierarchy meaning that a name can appear more than once as long as its under a different hierarchy. E.g. - `/root/A/X` and another `/root/B/X` is OK - `/root/A/X` and another `/root/A/X` is NOT OK Pull Request: https://projects.blender.org/blender/blender/pulls/135418 --- .../common/IO_abstract_hierarchy_iterator.h | 10 +- .../intern/abstract_hierarchy_iterator.cc | 33 +++++- tests/data | 2 +- tests/python/bl_usd_export_test.py | 104 ++++++++++++++++-- 4 files changed, 133 insertions(+), 16 deletions(-) diff --git a/source/blender/io/common/IO_abstract_hierarchy_iterator.h b/source/blender/io/common/IO_abstract_hierarchy_iterator.h index e3e940d6e29..dade895d2af 100644 --- a/source/blender/io/common/IO_abstract_hierarchy_iterator.h +++ b/source/blender/io/common/IO_abstract_hierarchy_iterator.h @@ -221,6 +221,9 @@ class AbstractHierarchyIterator { /* Mapping from ID to its export path. This is used for instancing; given an * instanced datablock, the export path of the original can be looked up. */ using ExportPathMap = blender::Map; + /* Mapping from ID name to a set of names logically residing "under" it. Used for unique + * name generation. */ + using ExportUsedNameMap = blender::Map>; /* IDs of all duplisource objects, used to identify instance prototypes. */ using DupliSources = blender::Set; @@ -232,6 +235,7 @@ class AbstractHierarchyIterator { WriterMap writers_; ExportSubset export_subset_; DupliSources duplisources_; + ExportUsedNameMap used_names_; public: explicit AbstractHierarchyIterator(Main *bmain, Depsgraph *depsgraph); @@ -258,6 +262,9 @@ class AbstractHierarchyIterator { * This base implementation is a no-op; override in a concrete subclass. */ virtual std::string make_valid_name(const std::string &name) const; + virtual std::string make_unique_name(const std::string &original_name, + Set &used_names); + /* Return the name of this ID datablock that is valid for the exported file format. Overriding is * only necessary if make_valid_name(id->name+2) is not suitable for the exported file format. * NULL-safe: when `id == nullptr` this returns an empty string. */ @@ -298,7 +305,8 @@ class AbstractHierarchyIterator { HierarchyContext context_for_object_data(const HierarchyContext *object_context) const; /* Convenience wrappers around get_id_name(). */ - std::string get_object_name(const Object *object) const; + std::string get_object_name(const Object *object); + std::string get_object_name(const Object *object, const Object *parent); std::string get_object_data_name(const Object *object) const; using create_writer_func = diff --git a/source/blender/io/common/intern/abstract_hierarchy_iterator.cc b/source/blender/io/common/intern/abstract_hierarchy_iterator.cc index a4f1d14a477..ea41963ae65 100644 --- a/source/blender/io/common/intern/abstract_hierarchy_iterator.cc +++ b/source/blender/io/common/intern/abstract_hierarchy_iterator.cc @@ -17,6 +17,8 @@ #include "BLI_assert.h" #include "BLI_listbase.h" #include "BLI_math_matrix.h" +#include "BLI_set.hh" +#include "BLI_string_utils.hh" #include "DNA_ID.h" #include "DNA_layer_types.h" @@ -206,6 +208,22 @@ std::string AbstractHierarchyIterator::get_id_name(const ID *id) const return make_valid_name(std::string(id->name + 2)); } +std::string AbstractHierarchyIterator::make_unique_name(const std::string &original_name, + Set &used_names) +{ + if (original_name.empty()) { + return ""; + } + + std::string name = BLI_uniquename_cb( + [&](const StringRef check_name) { return used_names.contains_as(check_name); }, + '_', + make_valid_name(original_name)); + + used_names.add_new(name); + return name; +} + std::string AbstractHierarchyIterator::get_object_data_path(const HierarchyContext *context) const { BLI_assert(!context->export_path.empty()); @@ -388,6 +406,7 @@ void AbstractHierarchyIterator::export_graph_clear() } } export_graph_.clear(); + used_names_.clear_and_keep_capacity(); } void AbstractHierarchyIterator::visit_object(Object *object, @@ -397,7 +416,7 @@ void AbstractHierarchyIterator::visit_object(Object *object, HierarchyContext *context = new HierarchyContext(); context->object = object; context->is_object_data_context = false; - context->export_name = get_object_name(object); + context->export_name = get_object_name(object, export_parent); context->export_parent = export_parent; context->duplicator = nullptr; context->weak_export = weak_export; @@ -450,7 +469,9 @@ void AbstractHierarchyIterator::visit_dupli_object(DupliObject *dupli_object, /* Construct export name for the dupli-instance. */ std::string export_name = get_object_name(context->object) + "-" + context->persistent_id.as_object_name_suffix(); - context->export_name = make_valid_name(export_name); + + Set &used_names = used_names_.lookup_or_add(duplicator->id.name, {}); + context->export_name = make_unique_name(make_valid_name(export_name), used_names); ObjectIdentifier graph_index = determine_graph_index_dupli( context, dupli_object, dupli_parent_finder); @@ -737,11 +758,17 @@ void AbstractHierarchyIterator::make_writers_particle_systems( } } -std::string AbstractHierarchyIterator::get_object_name(const Object *object) const +std::string AbstractHierarchyIterator::get_object_name(const Object *object) { return get_id_name(&object->id); } +std::string AbstractHierarchyIterator::get_object_name(const Object *object, const Object *parent) +{ + Set &used_names = used_names_.lookup_or_add(parent ? parent->id.name : "", {}); + return make_unique_name(object->id.name + 2, used_names); +} + std::string AbstractHierarchyIterator::get_object_data_name(const Object *object) const { ID *object_data = static_cast(object->data); diff --git a/tests/data b/tests/data index 51833b78c1c..90bc81396bb 160000 --- a/tests/data +++ b/tests/data @@ -1 +1 @@ -Subproject commit 51833b78c1c58c8619ca6caa6d100ddf52129187 +Subproject commit 90bc81396bb52e494715ffd2a90aaa11ca2b2ca6 diff --git a/tests/python/bl_usd_export_test.py b/tests/python/bl_usd_export_test.py index b75b3b9db2f..a246475d485 100644 --- a/tests/python/bl_usd_export_test.py +++ b/tests/python/bl_usd_export_test.py @@ -1258,15 +1258,11 @@ class USDExportTest(AbstractUSDTest): ("/root/Camera/Camera", "Camera"), ("/root/env_light", "DomeLight") ) - - def key(el): - return el[0] - - expected = tuple(sorted(expected, key=key)) + expected = tuple(sorted(expected, key=lambda pair: pair[0])) stage = Usd.Stage.Open(str(test_path)) actual = ((str(p.GetPath()), p.GetTypeName()) for p in stage.Traverse()) - actual = tuple(sorted(actual, key=key)) + actual = tuple(sorted(actual, key=lambda pair: pair[0])) self.assertTupleEqual(expected, actual) @@ -1311,14 +1307,11 @@ class USDExportTest(AbstractUSDTest): ("/root/env_light", "DomeLight") ) - def key(el): - return el[0] - - expected = tuple(sorted(expected, key=key)) + expected = tuple(sorted(expected, key=lambda pair: pair[0])) stage = Usd.Stage.Open(str(test_path)) actual = ((str(p.GetPath()), p.GetTypeName()) for p in stage.Traverse()) - actual = tuple(sorted(actual, key=key)) + actual = tuple(sorted(actual, key=lambda pair: pair[0])) self.assertTupleEqual(expected, actual) @@ -1490,6 +1483,95 @@ class USDExportTest(AbstractUSDTest): self.assertTrue(tex_path.exists(), f"Exported texture {tex_path} doesn't exist") + def test_naming_collision_hierarchy(self): + """Validate that naming collisions during export are handled correctly""" + bpy.ops.wm.open_mainfile(filepath=str(self.testdir / "usd_hierarchy_collision.blend")) + export_path = self.tempdir / "usd_hierarchy_collision.usda" + self.export_and_validate(filepath=str(export_path)) + + expected = ( + ('/root', 'Xform'), + ('/root/Empty', 'Xform'), + ('/root/Empty/Par_002', 'Xform'), + ('/root/Empty/Par_002/Par_1', 'Mesh'), + ('/root/Empty/Par_003', 'Xform'), + ('/root/Empty/Par_003/Par_1', 'Mesh'), + ('/root/Empty/Par_004', 'Xform'), + ('/root/Empty/Par_004/Par_002', 'Mesh'), + ('/root/Empty/Par_1', 'Xform'), + ('/root/Empty/Par_1/Par_1', 'Mesh'), + ('/root/Level1', 'Xform'), + ('/root/Level1/Level2', 'Xform'), + ('/root/Level1/Level2/Par2_002', 'Xform'), + ('/root/Level1/Level2/Par2_002/Par2_002', 'Mesh'), + ('/root/Level1/Level2/Par2_1', 'Xform'), + ('/root/Level1/Level2/Par2_1/Par2_1', 'Mesh'), + ('/root/Level1/Par2_002', 'Xform'), + ('/root/Level1/Par2_002/Par2_1', 'Mesh'), + ('/root/Level1/Par2_1', 'Xform'), + ('/root/Level1/Par2_1/Par2_1', 'Mesh'), + ('/root/Test_002', 'Xform'), + ('/root/Test_002/Test_1', 'Mesh'), + ('/root/Test_003', 'Xform'), + ('/root/Test_003/Test_1', 'Mesh'), + ('/root/Test_004', 'Xform'), + ('/root/Test_004/Test_002', 'Mesh'), + ('/root/Test_1', 'Xform'), + ('/root/Test_1/Test_1', 'Mesh'), + ('/root/env_light', 'DomeLight'), + ('/root/xSource_002', 'Xform'), + ('/root/xSource_002/Dup_002', 'Xform'), + ('/root/xSource_002/Dup_002/Dup_002', 'Mesh'), + ('/root/xSource_002/Dup_002_0', 'Xform'), + ('/root/xSource_002/Dup_002_0/Dup_002', 'Mesh'), + ('/root/xSource_002/Dup_002_1', 'Xform'), + ('/root/xSource_002/Dup_002_1/Dup_002', 'Mesh'), + ('/root/xSource_002/Dup_002_2', 'Xform'), + ('/root/xSource_002/Dup_002_2/Dup_002', 'Mesh'), + ('/root/xSource_002/Dup_002_3', 'Xform'), + ('/root/xSource_002/Dup_002_3/Dup_002', 'Mesh'), + ('/root/xSource_002/Dup_1', 'Xform'), + ('/root/xSource_002/Dup_1/Dup_1', 'Mesh'), + ('/root/xSource_002/Dup_1_0', 'Xform'), + ('/root/xSource_002/Dup_1_0/Dup_1', 'Mesh'), + ('/root/xSource_002/Dup_1_1', 'Xform'), + ('/root/xSource_002/Dup_1_1/Dup_1', 'Mesh'), + ('/root/xSource_002/Dup_1_2', 'Xform'), + ('/root/xSource_002/Dup_1_2/Dup_1', 'Mesh'), + ('/root/xSource_002/Dup_1_3', 'Xform'), + ('/root/xSource_002/Dup_1_3/Dup_1', 'Mesh'), + ('/root/xSource_002/xSource_1', 'Mesh'), + ('/root/xSource_1', 'Xform'), + ('/root/xSource_1/Dup_002', 'Xform'), + ('/root/xSource_1/Dup_002/Dup_1', 'Mesh'), + ('/root/xSource_1/Dup_1', 'Xform'), + ('/root/xSource_1/Dup_1/Dup_1', 'Mesh'), + ('/root/xSource_1/Dup_1_0', 'Xform'), + ('/root/xSource_1/Dup_1_0/Dup_1', 'Mesh'), + ('/root/xSource_1/Dup_1_001', 'Xform'), + ('/root/xSource_1/Dup_1_001/Dup_1', 'Mesh'), + ('/root/xSource_1/Dup_1_002', 'Xform'), + ('/root/xSource_1/Dup_1_002/Dup_1', 'Mesh'), + ('/root/xSource_1/Dup_1_003', 'Xform'), + ('/root/xSource_1/Dup_1_003/Dup_1', 'Mesh'), + ('/root/xSource_1/Dup_1_004', 'Xform'), + ('/root/xSource_1/Dup_1_004/Dup_1', 'Mesh'), + ('/root/xSource_1/Dup_1_1', 'Xform'), + ('/root/xSource_1/Dup_1_1/Dup_1', 'Mesh'), + ('/root/xSource_1/Dup_1_2', 'Xform'), + ('/root/xSource_1/Dup_1_2/Dup_1', 'Mesh'), + ('/root/xSource_1/Dup_1_3', 'Xform'), + ('/root/xSource_1/Dup_1_3/Dup_1', 'Mesh'), + ('/root/xSource_1/xSource_1', 'Mesh') + ) + expected = tuple(sorted(expected, key=lambda pair: pair[0])) + + stage = Usd.Stage.Open(str(export_path)) + actual = ((str(p.GetPath()), p.GetTypeName()) for p in stage.Traverse()) + actual = tuple(sorted(actual, key=lambda pair: pair[0])) + + self.assertTupleEqual(expected, actual) + class USDHookBase: instructions = {}