blender
/
blender
137
409
Fork 592
Code Issues 6.3k Pull Requests 771 Projects 19 Wiki Activity

Fix #105409: vertex interpolation corrupts Alembic mesh #105867

Closed
Kévin Dietrich wants to merge 4 commits from kevindietrich:abc_topology_fix_3.5 into blender-v3.6-release
pull from: kevindietrich:abc_topology_fix_3.5
merge into: blender:blender-v3.6-release

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 2 Commits 4 Files Changed 6 +77 -4
1 changed files with 77 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit b9b199aa5a - Show all commits

81
source/blender/io/alembic/intern/abc_reader_mesh.cc
View File

@ -463,6 +463,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;
brecht marked this conversation as resolved Outdated
Brecht Van Lommel commented 2023-03-18 00:07:40 +01:00
Outdated
Review
Copy Link

connexions -> connections

connexions -> connections
}
brecht marked this conversation as resolved Outdated
Brecht Van Lommel commented 2023-03-20 12:14:31 +01:00
Outdated
Review
Copy Link

Could the pointers be compared here as well to speed things up in case they are the same?

if (face_counts->get() != ceil_face_counts->get() &&
    memcmp(face_counts->get(), ceil_face_counts->get(), face_counts->size() * sizeof(int))) {
    return false;
}
Could the pointers be compared here as well to speed things up in case they are the same? ``` if (face_counts->get() != ceil_face_counts->get() && memcmp(face_counts->get(), ceil_face_counts->get(), face_counts->size() * sizeof(int))) { return false; } ```
Kévin Dietrich commented 2023-03-20 14:41:56 +01:00
Outdated
Review
Copy Link

I did try that originally but did not notice any difference with or without. Although, we could argue that not having an explicit check on our side is betting on the implementation of memcmp. I would venture that any serious implementation of memcmp would return 0 if the pointers are the same.

I did try that originally but did not notice any difference with or without. Although, we could argue that not having an explicit check on our side is betting on the implementation of memcmp. I would venture that any serious implementation of memcmp would return 0 if the pointers are the same.
Brecht Van Lommel commented 2023-03-20 14:49:04 +01:00
Outdated
Review
Copy Link

From this, it doesn't seem like something we should rely on:
https://stackoverflow.com/a/58305477

From this, it doesn't seem like something we should rely on: https://stackoverflow.com/a/58305477
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,
@ -481,7 +514,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) {
@ -666,9 +702,46 @@ 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. */
if (m_schema.getFaceIndicesProperty().getNumSamples() == 1 &&
m_schema.getFaceCountsProperty().getNumSamples() == 1)
{
return false;
brecht marked this conversation as resolved Outdated
Brecht Van Lommel commented 2023-03-20 12:11:47 +01:00
Outdated
Review
Copy Link

I guess this does not work for a sequence of Alembic files, only for a single file that contains the full animation? Or is that a different code path?

I guess this does not work for a sequence of Alembic files, only for a single file that contains the full animation? Or is that a different code path?
Kévin Dietrich commented 2023-03-20 14:43:34 +01:00
Outdated
Review
Copy Link

You're right, and there is no different code path for files sequences, so this may break; I did not think of that case. I do have some Alembic file sequences, so I can use those to correct the logic.

You're right, and there is no different code path for files sequences, so this may break; I did not think of that case. I do have some Alembic file sequences, so I can use those to correct the logic.
}
/* 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 loop_index = 0;
const OffsetIndices<int> &polys = existing_mesh->polys();
for (int i = 0; i < face_counts->size(); i++) {
const int face_size = (*face_counts)[i];
const IndexRange &poly = polys[i];
if (poly[i] != loop_index || poly.size() != face_size) {
return true;
}
/* NOTE: Alembic data is stored in the reverse order. */
uint rev_loop_index = loop_index + (face_size - 1);
for (int f = 0; f < face_size; f++, loop_index++, rev_loop_index--) {
if (poly[i] != ((*face_indices)[loop_index])) {
return true;
}
}
}
return false;
}
Mesh *AbcMeshReader::read_mesh(Mesh *existing_mesh,