Page MenuHome
Differential D9041

Fix T71981: Alembic override frame causes fluid sim mesh to have artifacts
ClosedPublic
Actions

Authored by Sybren A. Stüvel (sybren) on Sep 28 2020, 6:32 PM.

Details

Reviewers
Sergey Sharybin (sergey)
Bastien Montagne (mont29)
Maniphest Tasks
T71981: Alembic override frame causes fluid sim mesh to have artifacts
Commits
rBbab2260b59c7: Fix T71981: Alembic vertex interpolation can jumble mesh
Summary

Add an option to disable Alembic vertex interpolation.

Code-wise I'm quite confident that it's doing the right thing, but I wouldn't mind an extra pair of eyes. If possible, I'd like to have this in 2.91.

Alembic stores mesh samples at specific time keys; when a frame in
Blender maps to a timecode between two samples, Blender will interpolate
the mesh vertex positions. This interpolation only happens when the mesh
has a constant topology, but sometimes this is not detected properly
(when the vertices change order, but the number of mesh elements remains
the same) resulting in a jumbled up mesh (T71981). With this patch, users
have the ability to disable vertex interpolation.

An alternative would be to have better detection of topology changes,
but I'm afraid that that'll cause a considerable slowdown.

The versioning code will not be committed as-is, but will bump the
sub-version and move the existing versioning code into the appropriate
block.

Diff Detail

Repository
rB Blender

Event Timeline

Sybren A. Stüvel (sybren) created this revision.Sep 28 2020, 6:32 PM
Harbormaster completed remote builds in B10449: Diff 29346.Sep 28 2020, 6:32 PM
Sybren A. Stüvel (sybren) requested review of this revision.Sep 28 2020, 6:32 PM
Sybren A. Stüvel (sybren) added a reviewer: Bastien Montagne (mont29).Sep 28 2020, 6:32 PM
Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)
Sybren A. Stüvel (sybren) added projects: BF Blender (2.91), Alembic, Pipeline, Assets & I/O.
Sybren A. Stüvel (sybren) added a task: T71981: Alembic override frame causes fluid sim mesh to have artifacts.
Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)Sep 28 2020, 6:59 PM
Bastien Montagne (mont29) added a comment.Sep 28 2020, 9:27 PM

Not much to say, patch LGTM, but have you considered using an 'inverted' flag, like MOD_MESHSEQ_NO_VERTICES_INTERPOLATION? Can see two advantages to that:

  • No need of any do_version, since default value of the new flag (0) would be good.
  • In case in the future we manage to get a fully working heuristic to detect topology changes, it would also be easier to remove that option while preserving both forward and backward 'default' compatible value.
Sybren A. Stüvel (sybren) added a comment.Sep 29 2020, 10:23 AM
In D9041#222410, @Bastien Montagne (mont29) wrote:

Not much to say, patch LGTM, but have you considered using an 'inverted' flag, like MOD_MESHSEQ_NO_VERTICES_INTERPOLATION?

Yes I have considered that. I have a strong dislike for flags that need to be enabled to disable something. Too many negations make my head spin. The versioning code is only run "once", but the semantics of that flag will likely be with us for a long time.

Sergey Sharybin (sergey) requested changes to this revision.Sep 29 2020, 10:43 AM

I would prefer to have less negated flags, those are always confusing and bugprone.

However, there is one thing which is to be changed: the convention is to use use_ prefix for boolean flags in RNA. We don't use may_ prefix.

This revision now requires changes to proceed.Sep 29 2020, 10:43 AM
Sybren A. Stüvel (sybren) updated this revision to Diff 29369.Sep 29 2020, 2:46 PM
  • may_interpolate_vertices → use_vertex_interpolation
Harbormaster completed remote builds in B10461: Diff 29369.Sep 29 2020, 2:46 PM
Sybren A. Stüvel (sybren) updated this revision to Diff 29370.Sep 29 2020, 2:48 PM
  • Remove accidental formatting
Harbormaster completed remote builds in B10462: Diff 29370.Sep 29 2020, 2:48 PM
Sergey Sharybin (sergey) added a comment.Sep 29 2020, 2:59 PM

An alternative would be to have better detection of topology changes, but I'm afraid that that'll cause a considerable slowdown.

Doing full topology check is indeed something undesirable. However, is it possible to avoid crashes and instead show more meaningful information (using modifier's error message) ?

source/blender/io/alembic/intern/abc_reader_points.cc
154–157
Mesh *mesh_to_export = new_mesh ? new_mesh : existing_mesh;
const bool use_vertex_interpolation = read_flag & MOD_MESHSEQ_INTERPOLATE_VERTICES;
const CDStreamConfig config = get_config(mesh_to_export, use_vertex_interpolation);

...

return mesh_to_export;

But sure, other than the boolean flag the rest is the existing code.

The point of adding extra boolean variable is to avoid inlined bit operations in the function call, which, I think, is more readable.

Sybren A. Stüvel (sybren) updated this revision to Diff 29371.Sep 29 2020, 3:35 PM
  • Move bit extraction to separate const bool
Harbormaster completed remote builds in B10463: Diff 29371.Sep 29 2020, 3:36 PM
Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)Sep 29 2020, 3:44 PM
Sergey Sharybin (sergey) accepted this revision.Sep 29 2020, 3:45 PM

After discussion in the chat, there is no crash and no corruption in my initial understanding (topology is valid, but order of vertices is different, so the result is becoming garbage, but no crashes or anything).

Maybe meaning of corruption can be clarified in the final commit message, but the code wise it all seems fain to me.

This revision is now accepted and ready to land.Sep 29 2020, 3:45 PM
Sybren A. Stüvel (sybren) updated this revision to Diff 29372.Sep 29 2020, 3:55 PM
  • Moved determining which mesh to export out of function arguments
Harbormaster completed remote builds in B10464: Diff 29372.Sep 29 2020, 3:55 PM
Closed by commit rBbab2260b59c7: Fix T71981: Alembic vertex interpolation can jumble mesh (authored by Sybren A. Stüvel (sybren)). · Explain WhySep 29 2020, 4:13 PM
This revision was automatically updated to reflect the committed changes.
Sybren A. Stüvel (sybren) added a commit: rBbab2260b59c7: Fix T71981: Alembic vertex interpolation can jumble mesh.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
2 lines
blenloader/
intern/
38 lines
io/
alembic/
intern/
1 line
2 lines
17 lines
9 lines
makesdna/
5 lines
makesrna/
intern/
5 lines
modifiers/
intern/
3 lines

Diff 29375

View Options

source/blender/blenkernel/BKE_blender_version.h

Loading...
View Options

source/blender/blenloader/intern/versioning_290.c

Loading...
View Options

source/blender/io/alembic/intern/abc_customdata.h

Loading...
View Options

source/blender/io/alembic/intern/abc_reader_mesh.h

Loading...
View Options

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

Loading...
View Options

source/blender/io/alembic/intern/abc_reader_points.cc

Loading...
View Options

source/blender/makesdna/DNA_modifier_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_modifier.c

Loading...
View Options

source/blender/modifiers/intern/MOD_meshsequencecache.c

Loading...