From cbb6cdf8d74044a6a5417cdcc47748a71dd9ab70 Mon Sep 17 00:00:00 2001 From: Jesse Yurkovich Date: Mon, 14 Jul 2025 21:49:23 +0200 Subject: [PATCH] Fix #141633: Various subdivision problems during Alembic import This fixes 4 bugs all conspiring to make the referenced SubD scenario quite broken: - When manually creating a MeshSequenceCache, the reading of edge and vertex crease data was skipped. Fixed by reading crease data inside the common `read_mesh` method. - When importing an Alembic with animated edge or vertex crease data, a MeshSequenceCache modifier was not being added to the object. This was due to not checking the relevant crease properties and required adding a specialized `has_animations` function. - When importing animated vertex crease data, a duplicate `vertex_crease` attribute would be created, breaking the animation. Fixed by using the attribute API rather than custom data. - The MeshSequenceCache scenario would call into the Alembic Mesh reader which ended up referencing deallocated stack memory for the ImportSettings. In release builds this would cause sporadic failures because the value of `blender_archive_version_prior_44` would be random. There was already a very old TODO for this and we finally really needed to address it. A new test was added which exports animated creases on two meshes and re-imports them back in. It verifies that each mesh gets a MeshSequenceCache modifier added and also ensures the values of the creases are correct for all frames. Pull Request: https://projects.blender.org/blender/blender/pulls/141646 --- .../io/alembic/intern/abc_reader_mesh.cc | 71 +++++++++++++------ .../io/alembic/intern/abc_reader_object.h | 4 +- .../blender/io/alembic/intern/alembic_capi.cc | 42 ++++++++--- tests/files/alembic/mesh_subd_varying.blend | 3 + tests/python/bl_alembic_io_test.py | 57 +++++++++++++++ 5 files changed, 147 insertions(+), 30 deletions(-) create mode 100644 tests/files/alembic/mesh_subd_varying.blend diff --git a/source/blender/io/alembic/intern/abc_reader_mesh.cc b/source/blender/io/alembic/intern/abc_reader_mesh.cc index 9c043e38d28..1d1a1b6c276 100644 --- a/source/blender/io/alembic/intern/abc_reader_mesh.cc +++ b/source/blender/io/alembic/intern/abc_reader_mesh.cc @@ -573,6 +573,43 @@ template<> bool has_animations(Alembic::AbcGeom::IPolyMeshSchema &schema, Import return false; } +template<> bool has_animations(Alembic::AbcGeom::ISubDSchema &schema, ImportSettings *settings) +{ + if (settings->is_sequence || !schema.isConstant()) { + return true; + } + + IV2fGeomParam uvsParam = schema.getUVsParam(); + if (uvsParam.valid() && !uvsParam.isConstant()) { + return true; + } + + Alembic::AbcGeom::IInt32ArrayProperty creaseIndices = schema.getCreaseIndicesProperty(); + if (creaseIndices.valid() && !creaseIndices.isConstant()) { + return true; + } + Alembic::AbcGeom::IFloatArrayProperty creaseSharpnesses = schema.getCreaseSharpnessesProperty(); + if (creaseSharpnesses.valid() && !creaseSharpnesses.isConstant()) { + return true; + } + + Alembic::AbcGeom::IInt32ArrayProperty cornerIndices = schema.getCornerIndicesProperty(); + if (cornerIndices.valid() && !cornerIndices.isConstant()) { + return true; + } + Alembic::AbcGeom::IFloatArrayProperty cornerSharpnesses = schema.getCornerSharpnessesProperty(); + if (cornerSharpnesses.valid() && !cornerSharpnesses.isConstant()) { + return true; + } + + ICompoundProperty arbGeomParams = schema.getArbGeomParams(); + if (has_animated_geom_params(arbGeomParams)) { + return true; + } + + return false; +} + void AbcMeshReader::readObjectData(Main *bmain, const Alembic::Abc::ISampleSelector &sample_sel) { Mesh *mesh = BKE_mesh_add(bmain, m_data_name.c_str()); @@ -922,8 +959,11 @@ static void read_vertex_creases(Mesh *mesh, return; } - float *vertex_crease_data = (float *)CustomData_add_layer_named( - &mesh->vert_data, CD_PROP_FLOAT, CD_SET_DEFAULT, mesh->verts_num, "crease_vert"); + bke::MutableAttributeAccessor attributes = mesh->attributes_for_write(); + bke::SpanAttributeWriter creases = attributes.lookup_or_add_for_write_only_span( + "crease_vert", bke::AttrDomain::Point); + creases.span.fill(0.0f); + const int totvert = mesh->verts_num; for (int i = 0, v = indices->size(); i < v; ++i) { @@ -936,8 +976,10 @@ static void read_vertex_creases(Mesh *mesh, const float crease = settings->blender_archive_version_prior_44 ? (*sharpnesses)[i] : bke::subdiv::sharpness_to_crease((*sharpnesses)[i]); - vertex_crease_data[idx] = std::clamp(crease, 0.0f, 1.0f); + creases.span[idx] = std::clamp(crease, 0.0f, 1.0f); } + + creases.finish(); } static void read_edge_creases(Mesh *mesh, @@ -1027,23 +1069,6 @@ void AbcSubDReader::readObjectData(Main *bmain, const Alembic::Abc::ISampleSelec BKE_mesh_nomain_to_mesh(read_mesh, mesh, m_object); } - ISubDSchema::Sample sample; - try { - sample = m_schema.getValue(sample_sel); - } - catch (Alembic::Util::Exception &ex) { - printf("Alembic: error reading mesh sample for '%s/%s' at time %f: %s\n", - m_iobject.getFullName().c_str(), - m_schema.getName().c_str(), - sample_sel.getRequestedTime(), - ex.what()); - return; - } - - read_edge_creases(mesh, sample.getCreaseIndices(), sample.getCreaseSharpnesses(), m_settings); - - read_vertex_creases(mesh, sample.getCornerIndices(), sample.getCornerSharpnesses(), m_settings); - if (m_settings->validate_meshes) { BKE_mesh_validate(mesh, false, false); } @@ -1117,6 +1142,12 @@ Mesh *AbcSubDReader::read_mesh(Mesh *existing_mesh, config.modifier_error_message = r_err_str; read_subd_sample(m_iobject.getFullName(), &settings, m_schema, sample_sel, config); + read_edge_creases( + mesh_to_export, sample.getCreaseIndices(), sample.getCreaseSharpnesses(), m_settings); + + read_vertex_creases( + mesh_to_export, sample.getCornerIndices(), sample.getCornerSharpnesses(), m_settings); + return mesh_to_export; } diff --git a/source/blender/io/alembic/intern/abc_reader_object.h b/source/blender/io/alembic/intern/abc_reader_object.h index 9ba0f0f87ad..1fc16072c6f 100644 --- a/source/blender/io/alembic/intern/abc_reader_object.h +++ b/source/blender/io/alembic/intern/abc_reader_object.h @@ -73,7 +73,9 @@ class AbcObjectReader { Object *m_object; Alembic::Abc::IObject m_iobject; - /* XXX - TODO(kevindietrich) : this references stack memory... */ + /* XXX - This used to reference stack memory for MeshSequenceCache scenarios. That has been + * addressed but ownership of these settings should be made more apparent to prevent similar + * issues in the future. */ ImportSettings *m_settings; /* This is initialized from the ImportSettings above on construction. It will need to be removed * once we fix the stack memory reference situation. */ diff --git a/source/blender/io/alembic/intern/alembic_capi.cc b/source/blender/io/alembic/intern/alembic_capi.cc index c14f9dbead4..1e386edd5bf 100644 --- a/source/blender/io/alembic/intern/alembic_capi.cc +++ b/source/blender/io/alembic/intern/alembic_capi.cc @@ -79,12 +79,27 @@ using Alembic::AbcMaterial::IMaterial; using namespace blender::io::alembic; -BLI_INLINE ArchiveReader *archive_from_handle(CacheArchiveHandle *handle) +struct AlembicArchiveData { + ArchiveReader *archive_reader = nullptr; + ImportSettings *settings = nullptr; + + AlembicArchiveData() = default; + ~AlembicArchiveData() + { + delete archive_reader; + delete settings; + } + + AlembicArchiveData(const AlembicArchiveData &) = delete; + AlembicArchiveData &operator==(const AlembicArchiveData &) = delete; +}; + +BLI_INLINE AlembicArchiveData *archive_from_handle(CacheArchiveHandle *handle) { - return reinterpret_cast(handle); + return reinterpret_cast(handle); } -BLI_INLINE CacheArchiveHandle *handle_from_archive(ArchiveReader *archive) +BLI_INLINE CacheArchiveHandle *handle_from_archive(AlembicArchiveData *archive) { return reinterpret_cast(archive); } @@ -181,7 +196,11 @@ CacheArchiveHandle *ABC_create_handle(const Main *bmain, gather_objects_paths(archive->getTop(), object_paths); } - return handle_from_archive(archive); + AlembicArchiveData *archive_data = new AlembicArchiveData(); + archive_data->archive_reader = archive; + archive_data->settings = new ImportSettings(); + + return handle_from_archive(archive_data); } void ABC_free_handle(CacheArchiveHandle *handle) @@ -888,8 +907,12 @@ CacheReader *CacheReader_open_alembic_object(CacheArchiveHandle *handle, return reader; } - ArchiveReader *archive = archive_from_handle(handle); + AlembicArchiveData *archive_data = archive_from_handle(handle); + if (!archive_data) { + return reader; + } + ArchiveReader *archive = archive_data->archive_reader; if (!archive || !archive->valid()) { return reader; } @@ -901,10 +924,11 @@ CacheReader *CacheReader_open_alembic_object(CacheArchiveHandle *handle, ABC_CacheReader_free(reader); } - ImportSettings settings; - settings.is_sequence = is_sequence; - settings.blender_archive_version_prior_44 = archive->is_blender_archive_version_prior_44(); - AbcObjectReader *abc_reader = create_reader(iobject, settings); + archive_data->settings->is_sequence = is_sequence; + archive_data->settings->blender_archive_version_prior_44 = + archive->is_blender_archive_version_prior_44(); + + AbcObjectReader *abc_reader = create_reader(iobject, *archive_data->settings); if (abc_reader == nullptr) { /* This object is not supported */ return nullptr; diff --git a/tests/files/alembic/mesh_subd_varying.blend b/tests/files/alembic/mesh_subd_varying.blend new file mode 100644 index 00000000000..ce826f92fd7 --- /dev/null +++ b/tests/files/alembic/mesh_subd_varying.blend @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:9d9613c315deb164323eb6eb74cae62119fad46d64b0ea950331afdd4efa7e9c +size 97884 diff --git a/tests/python/bl_alembic_io_test.py b/tests/python/bl_alembic_io_test.py index b10d3b78e6f..5fcd9b13608 100644 --- a/tests/python/bl_alembic_io_test.py +++ b/tests/python/bl_alembic_io_test.py @@ -277,6 +277,63 @@ class CameraExportImportTest(unittest.TestCase): for name in self.names: self.assertIsNone(objects[name].parent) + def test_mesh_subd_varying(self): + """Test meshes with subdivision crease values varying over time.""" + + abc_path = str(self.tempdir / "mesh_subd_varying.abc") + + # Export + bpy.ops.wm.open_mainfile(filepath=str(args.testdir / "mesh_subd_varying.blend")) + self.assertIn('FINISHED', bpy.ops.wm.alembic_export( + filepath=abc_path, + subdiv_schema=True + )) + + # Re-import what we just exported into an empty file. + bpy.ops.wm.open_mainfile(filepath=str(args.testdir / "empty.blend")) + bpy.ops.wm.alembic_import( + filepath=abc_path, + as_background_job=False) + + # + # Validate Mesh data + # + blender_mesh1 = bpy.data.objects["mesh_edge_crease"] + blender_mesh2 = bpy.data.objects["mesh_vert_crease"] + + # A MeshSequenceCache modifier should be present on every imported object + for blender_mesh in [blender_mesh1, blender_mesh2]: + self.assertTrue(len(blender_mesh.modifiers) == 1 and blender_mesh.modifiers[0].type == + 'MESH_SEQUENCE_CACHE', f"{blender_mesh.name} has incorrect modifiers") + + # Conversion from USD to Blender convention + def sharpness_to_crease(sharpness): + return math.sqrt(sharpness * 0.1) + + # Compare Blender data against the expected value for every frame + for frame in range(1, 25): + bpy.context.scene.frame_set(frame) + depsgraph = bpy.context.evaluated_depsgraph_get() + blender_mesh1_eval = bpy.data.objects["mesh_edge_crease"].evaluated_get(depsgraph) + blender_mesh2_eval = bpy.data.objects["mesh_vert_crease"].evaluated_get(depsgraph) + + # The file was written using a simple formula for each frame's crease value + expected_edge_creases = [round(frame / 24.0, 3)] * 12 + expected_vert_creases = [round(frame / 24.0, 3)] * 4 + + # Check crease values + blender_crease_data = [round(d.value, 3) for d in blender_mesh1_eval.data.attributes["crease_edge"].data] + self.assertEqual( + blender_crease_data, + expected_edge_creases, + f"Frame {frame}: {blender_mesh1_eval.name} crease values do not match") + + blender_crease_data = [round(d.value, 3) for d in blender_mesh2_eval.data.attributes["crease_vert"].data] + self.assertEqual( + blender_crease_data, + expected_vert_creases, + f"Frame {frame}: {blender_mesh2_eval.name} crease values do not match") + def do_export_import_test(self, *, flatten: bool): bpy.ops.wm.open_mainfile(filepath=str(args.testdir / "camera_transforms.blend"))