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

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Kévin Dietrich commented

2023-03-17 23:25:54 +01:00

Member

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.

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.

Kévin Dietrich added the

label 2023-03-17 23:25:54 +01:00

Kévin Dietrich requested review from Sybren A. Stüvel 2023-03-17 23:26:18 +01:00

Brecht Van Lommel commented

2023-03-18 00:27:40 +01:00

Owner

I would guess Alembic does not store multiples samples of face counts and indices if the topology is the same for the entire animation? If so, this could avoid the overhead of comparison when .getNumSamples() == 1. Or maybe it could check if the array data pointers are equal.

I would guess Alembic does not store multiples samples of face counts and indices if the topology is the same for the entire animation? If so, this could avoid the overhead of comparison when `.getNumSamples() == 1`. Or maybe it could check if the array data pointers are equal.

Brecht Van Lommel reviewed 2023-03-18 00:28:10 +01:00

source/blender/io/alembic/intern/abc_reader_mesh.cc Outdated

						
				@ -469,0 +486,4 @@

				  }

				  /* Otherwise, we need to check the connectivity as files from e.g. videogrammetry may have the

				   * same face count, but different connexions between faces. */

Brecht Van Lommel commented

2023-03-18 00:07:40 +01:00

Owner

connexions -> connections

connexions -> connections

brecht marked this conversation as resolved

source/blender/io/alembic/intern/abc_reader_mesh.cc Outdated

						
				@ -469,0 +502,4 @@

				        return false;

				      }

				    }

				  }

Brecht Van Lommel commented

2023-03-18 00:12:55 +01:00

Owner

memcmp of the arrays would be a bit faster?

`memcmp` of the arrays would be a bit faster?

brecht marked this conversation as resolved

Brecht Van Lommel requested changes 2023-03-20 12:15:05 +01:00

source/blender/io/alembic/intern/abc_reader_mesh.cc Outdated

						
				@ -469,0 +488,4 @@

				  /* 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))) {

Brecht Van Lommel commented

2023-03-20 12:14:31 +01:00

Owner

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; } ```

brecht marked this conversation as resolved

source/blender/io/alembic/intern/abc_reader_mesh.cc Outdated

						
				@ -675,0 +714,4 @@

				  }

				  /* Check first if we indeed have multiple samples. */

				  if (m_schema.getFaceIndicesProperty().getNumSamples() == 1 &&

Brecht Van Lommel commented

2023-03-20 12:11:47 +01:00

Owner

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?

brecht marked this conversation as resolved

Kévin Dietrich reviewed 2023-03-20 14:47:02 +01:00

source/blender/io/alembic/intern/abc_reader_mesh.cc Outdated

						
				@ -469,0 +488,4 @@

				  /* 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))) {

Kévin Dietrich commented

2023-03-20 14:41:56 +01:00

Author

Member

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 marked this conversation as resolved

source/blender/io/alembic/intern/abc_reader_mesh.cc Outdated

						
				@ -469,3 +501,4 @@

				static void read_mesh_sample(const std::string &iobject_full_name,

				                             ImportSettings *settings,

				                             const IPolyMeshSchema &schema,

Kévin Dietrich commented

2023-03-18 01:07:38 +01:00

Author

Member

Just to note, memcmp gives about 1-2 fps speedup in the file from the bug report (~150k faces).

Just to note, `memcmp` gives about 1-2 fps speedup in the file from the bug report (~150k faces).

brecht marked this conversation as resolved

source/blender/io/alembic/intern/abc_reader_mesh.cc Outdated

						
				@ -675,0 +714,4 @@

				  }

				  /* Check first if we indeed have multiple samples. */

				  if (m_schema.getFaceIndicesProperty().getNumSamples() == 1 &&

Kévin Dietrich commented

2023-03-20 14:43:34 +01:00

Author

Member

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.

brecht marked this conversation as resolved

Brecht Van Lommel reviewed 2023-03-20 14:49:50 +01:00

source/blender/io/alembic/intern/abc_reader_mesh.cc Outdated

						
				@ -469,0 +488,4 @@

				  /* 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))) {

Brecht Van Lommel commented

2023-03-20 14:49:04 +01:00

Owner

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

brecht marked this conversation as resolved

Sybren A. Stüvel approved these changes 2023-04-17 16:52:18 +02:00

Sybren A. Stüvel left a comment

Member

Apart from what @brecht already said, the PR looks good to me.
Accepting so that the patch can be landed once Brecht also approves.

Apart from what @brecht already said, the PR looks good to me. Accepting so that the patch can be landed once Brecht also approves.

Kévin Dietrich force-pushed abc_topology_fix_3.5 from 1357054eeb to e8ea03e793

2023-06-14 13:13:19 +02:00

Compare

Kévin Dietrich changed title from ~~Fix #105409: vertex interpolation corrupts Alembic mesh~~ to Fix #105409: vertex interpolation corrupts Alembic mesh

2023-06-14 13:13:32 +02:00

kevindietrich changed target branch from blender-v3.5-release to blender-v3.6-release

2023-06-14 13:13:43 +02:00

Kévin Dietrich changed title from ~~Fix #105409: vertex interpolation corrupts Alembic mesh~~ to Fix #105409: vertex interpolation corrupts Alembic mesh

2023-06-14 13:13:55 +02:00

Kévin Dietrich commented

2023-06-14 13:14:28 +02:00

Author

Member

Sorry for the delayed update.

If a file sequence is being read, then we do a full topology comparison.

In order to detect if a file sequence is read, a new member is added to the base AbcObjectReader class, which is initialized when constructing a new reader. Technically, this information is alredy stored in ImportSettings which is already stored in the class, however, it is a pointer to some stack memory (either the stack from the import job, or the stack from CacheReader_open_alembic_object). This will need to be solved at some point (some of my other patches will remove the need for this object). CacheReader_open_alembic_object also had to be modified to take an extra parameter to tell if the CacheFile is a sequence.

Sorry for the delayed update. If a file sequence is being read, then we do a full topology comparison. In order to detect if a file sequence is read, a new member is added to the base AbcObjectReader class, which is initialized when constructing a new reader. Technically, this information is alredy stored in ImportSettings which is already stored in the class, however, it is a pointer to some stack memory (either the stack from the import job, or the stack from CacheReader_open_alembic_object). This will need to be solved at some point (some of my other patches will remove the need for this object). CacheReader_open_alembic_object also had to be modified to take an extra parameter to tell if the CacheFile is a sequence.

Brecht Van Lommel approved these changes 2023-06-14 13:18:46 +02:00

Brecht Van Lommel left a comment

Owner

Thanks, looks good now.

Thanks, looks good now.

Kévin Dietrich referenced this issue from a commit

2023-06-15 03:40:28 +02:00

Fix #105409: vertex interpolation corrupts Alembic mesh

Kévin Dietrich closed this pull request

2023-06-15 03:41:54 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#105867

No description provided.