From ae5fc2a7f82e4d17e8300a3532d0943cff3f3ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dietrich?= Date: Thu, 15 Jun 2023 03:37:29 +0200 Subject: [PATCH] Fix #105409: vertex interpolation corrupts Alembic mesh The Alembic data streaming can optionally interpolate between vertex of two adjacent frames in order to smooth out the transition between frames. However, the decision to interpolate is only based on the vertex count. This is not too robust as topology/connectivity can still differ even if the number of vertices is the same (for example physics simulations and videogrammetry can be set to output the same vertex count, but optimize the triangle placement). This lead to vertices of unrelated polygons being interpolated across frames. To fix this, we now also check if the connectivity across frames is the same, instead of just checking the topology counters. Although the bug is revealed by the vertex interpolation routine, a similar fix is applied to the check on topology change used to decide if the modifier has to be evaluated for orco evaluation. Pull Request: #105867 --- source/blender/blenkernel/intern/cachefile.c | 3 +- source/blender/io/alembic/ABC_alembic.h | 3 +- .../io/alembic/intern/abc_reader_mesh.cc | 82 ++++++++++++++++++- .../io/alembic/intern/abc_reader_object.cc | 1 + .../io/alembic/intern/abc_reader_object.h | 4 + .../blender/io/alembic/intern/alembic_capi.cc | 4 +- 6 files changed, 90 insertions(+), 7 deletions(-) diff --git a/source/blender/blenkernel/intern/cachefile.c b/source/blender/blenkernel/intern/cachefile.c index 3050945075e..b0284614b48 100644 --- a/source/blender/blenkernel/intern/cachefile.c +++ b/source/blender/blenkernel/intern/cachefile.c @@ -188,7 +188,8 @@ void BKE_cachefile_reader_open(CacheFile *cache_file, case CACHEFILE_TYPE_ALEMBIC: # ifdef WITH_ALEMBIC /* Open Alembic cache reader. */ - *reader = CacheReader_open_alembic_object(cache_file->handle, *reader, object, object_path); + *reader = CacheReader_open_alembic_object( + cache_file->handle, *reader, object, object_path, cache_file->is_sequence); # endif break; case CACHEFILE_TYPE_USD: diff --git a/source/blender/io/alembic/ABC_alembic.h b/source/blender/io/alembic/ABC_alembic.h index 05025861857..e55aec30270 100644 --- a/source/blender/io/alembic/ABC_alembic.h +++ b/source/blender/io/alembic/ABC_alembic.h @@ -141,7 +141,8 @@ void ABC_CacheReader_free(struct CacheReader *reader); struct CacheReader *CacheReader_open_alembic_object(struct CacheArchiveHandle *handle, struct CacheReader *reader, struct Object *object, - const char *object_path); + const char *object_path, + bool is_sequence); #ifdef __cplusplus } diff --git a/source/blender/io/alembic/intern/abc_reader_mesh.cc b/source/blender/io/alembic/intern/abc_reader_mesh.cc index 63222ccce35..781369a8816 100644 --- a/source/blender/io/alembic/intern/abc_reader_mesh.cc +++ b/source/blender/io/alembic/intern/abc_reader_mesh.cc @@ -465,6 +465,39 @@ static void read_velocity(const V3fArraySamplePtr &velocities, } } +static bool samples_have_same_topology(const IPolyMeshSchema::Sample &sample, + const IPolyMeshSchema::Sample &ceil_sample) +{ + const P3fArraySamplePtr &positions = sample.getPositions(); + const Alembic::Abc::Int32ArraySamplePtr &face_indices = sample.getFaceIndices(); + const Alembic::Abc::Int32ArraySamplePtr &face_counts = sample.getFaceCounts(); + + const P3fArraySamplePtr &ceil_positions = ceil_sample.getPositions(); + const Alembic::Abc::Int32ArraySamplePtr &ceil_face_indices = ceil_sample.getFaceIndices(); + const Alembic::Abc::Int32ArraySamplePtr &ceil_face_counts = ceil_sample.getFaceCounts(); + + /* It the counters are different, we can be sure the topology is different. */ + const bool different_counters = positions->size() != ceil_positions->size() || + face_counts->size() != ceil_face_counts->size() || + face_indices->size() != ceil_face_indices->size(); + if (different_counters) { + return false; + } + + /* Otherwise, we need to check the connectivity as files from e.g. videogrammetry may have the + * same face count, but different connections between faces. */ + + if (memcmp(face_counts->get(), ceil_face_counts->get(), face_counts->size() * sizeof(int))) { + return false; + } + + if (memcmp(face_indices->get(), ceil_face_indices->get(), face_indices->size() * sizeof(int))) { + return false; + } + + return true; +} + static void read_mesh_sample(const std::string &iobject_full_name, ImportSettings *settings, const IPolyMeshSchema &schema, @@ -483,7 +516,10 @@ static void read_mesh_sample(const std::string &iobject_full_name, if (config.weight != 0.0f) { Alembic::AbcGeom::IPolyMeshSchema::Sample ceil_sample; schema.get(ceil_sample, Alembic::Abc::ISampleSelector(config.ceil_index)); - abc_mesh_data.ceil_positions = ceil_sample.getPositions(); + if (samples_have_same_topology(sample, ceil_sample)) { + /* Only set interpolation data if the samples are compatible. */ + abc_mesh_data.ceil_positions = ceil_sample.getPositions(); + } } if ((settings->read_flag & MOD_MESHSEQ_READ_UV) != 0) { @@ -668,9 +704,47 @@ bool AbcMeshReader::topology_changed(const Mesh *existing_mesh, const ISampleSel const Alembic::Abc::Int32ArraySamplePtr &face_indices = sample.getFaceIndices(); const Alembic::Abc::Int32ArraySamplePtr &face_counts = sample.getFaceCounts(); - return positions->size() != existing_mesh->totvert || - face_counts->size() != existing_mesh->totpoly || - face_indices->size() != existing_mesh->totloop; + /* It the counters are different, we can be sure the topology is different. */ + const bool different_counters = positions->size() != existing_mesh->totvert || + face_counts->size() != existing_mesh->totpoly || + face_indices->size() != existing_mesh->totloop; + if (different_counters) { + return true; + } + + /* Check first if we indeed have multiple samples, unless we read a file sequence in which case + * we need to do a full topology comparison. */ + if (!m_is_reading_a_file_sequence && (m_schema.getFaceIndicesProperty().getNumSamples() == 1 && + m_schema.getFaceCountsProperty().getNumSamples() == 1)) + { + return false; + } + + /* Otherwise, we need to check the connectivity as files from e.g. videogrammetry may have the + * same face count, but different connections between faces. */ + uint abc_index = 0; + + const int *mesh_corners = existing_mesh->corner_verts().data(); + const int *mesh_poly_offsets = existing_mesh->poly_offsets().data(); + + for (int i = 0; i < face_counts->size(); i++) { + if (mesh_poly_offsets[i] != abc_index) { + return true; + } + + const int abc_face_size = (*face_counts)[i]; + /* NOTE: Alembic data is stored in the reverse order. */ + uint rev_loop_index = abc_index + (abc_face_size - 1); + for (int f = 0; f < abc_face_size; f++, abc_index++, rev_loop_index--) { + const int mesh_vert = mesh_corners[rev_loop_index]; + const int abc_vert = (*face_indices)[abc_index]; + if (mesh_vert != abc_vert) { + return true; + } + } + } + + return false; } Mesh *AbcMeshReader::read_mesh(Mesh *existing_mesh, diff --git a/source/blender/io/alembic/intern/abc_reader_object.cc b/source/blender/io/alembic/intern/abc_reader_object.cc index 3059cfe01a4..ed36cf6a715 100644 --- a/source/blender/io/alembic/intern/abc_reader_object.cc +++ b/source/blender/io/alembic/intern/abc_reader_object.cc @@ -33,6 +33,7 @@ AbcObjectReader::AbcObjectReader(const IObject &object, ImportSettings &settings : m_object(nullptr), m_iobject(object), m_settings(&settings), + m_is_reading_a_file_sequence(settings.is_sequence), m_min_time(std::numeric_limits::max()), m_max_time(std::numeric_limits::min()), m_refcount(0), diff --git a/source/blender/io/alembic/intern/abc_reader_object.h b/source/blender/io/alembic/intern/abc_reader_object.h index 6c57f32045b..d38326f7eb7 100644 --- a/source/blender/io/alembic/intern/abc_reader_object.h +++ b/source/blender/io/alembic/intern/abc_reader_object.h @@ -77,7 +77,11 @@ class AbcObjectReader { Object *m_object; Alembic::Abc::IObject m_iobject; + /* XXX - TODO(kevindietrich) : this references stack memory... */ ImportSettings *m_settings; + /* This is initialised from the ImportSettings above on construction. It will need to be removed + * once we fix the stack memory reference situation. */ + bool m_is_reading_a_file_sequence = false; chrono_t m_min_time; chrono_t m_max_time; diff --git a/source/blender/io/alembic/intern/alembic_capi.cc b/source/blender/io/alembic/intern/alembic_capi.cc index f466f9536d4..1962411a0d9 100644 --- a/source/blender/io/alembic/intern/alembic_capi.cc +++ b/source/blender/io/alembic/intern/alembic_capi.cc @@ -847,7 +847,8 @@ void ABC_CacheReader_incref(CacheReader *reader) CacheReader *CacheReader_open_alembic_object(CacheArchiveHandle *handle, CacheReader *reader, Object *object, - const char *object_path) + const char *object_path, + const bool is_sequence) { if (object_path[0] == '\0') { return reader; @@ -867,6 +868,7 @@ CacheReader *CacheReader_open_alembic_object(CacheArchiveHandle *handle, } ImportSettings settings; + settings.is_sequence = is_sequence; AbcObjectReader *abc_reader = create_reader(iobject, settings); if (abc_reader == nullptr) { /* This object is not supported */