Page MenuHome

Fix T81330: Alembic Import not adding modifiers to constant meshes with animated vertex colors
ClosedPublic

Authored by Philipp Oeser (lichtwerk) on Sep 30 2020, 7:11 PM.

Details

Summary

If the mesh was constant, no check was done if there were animated
vertex colors and thus creation of a MeshSequenceCache modifier was
skipped.

Diff Detail

Repository
rB Blender

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.Sep 30 2020, 7:11 PM
Philipp Oeser (lichtwerk) created this revision.
Philipp Oeser (lichtwerk) retitled this revision from Fix T81330: Alembic Import ignores constant meshes with animated vertex colors to Fix T81330: Alembic Import not adding modifiers to constant meshes with animated vertex colors.Sep 30 2020, 7:16 PM
Philipp Oeser (lichtwerk) planned changes to this revision.Sep 30 2020, 7:21 PM

Need to fix a crash in a certain situation.

fix creating wrong param type

Note that the missing writing of animated vertex colors already has its own task in T51488: Alembic export animated vertex color exports only first frame vertex color

return early if possible

Sybren A. Stüvel (sybren) requested changes to this revision.Oct 1 2020, 12:03 PM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/io/alembic/intern/abc_reader_mesh.cc
550

Does this always return a valid ICompoundProperty?

550–567

This should be its own function.

555–566

The code in both if block is exactly the same, except for the variable type. This can be abstracted into a templated function.

This revision now requires changes to proceed.Oct 1 2020, 12:03 PM

address review comments

Philipp Oeser (lichtwerk) marked 3 inline comments as done.Oct 1 2020, 1:44 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Oct 1 2020, 2:24 PM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/io/alembic/intern/abc_reader_mesh.cc
503–510

Here you can flip the conditions and return early.

if (!typedGeomParam::matches(prop_header)) {
  return false;
}

typedGeomParam color_param(arbGeomParams, prop_header.getName());
return color_param.valid() && !color_param.isConstant();
504

Given the parameters, it's no longer given that this is related to colour at all. Probably it's better to rename color_param to geom_param.

513

has_animated_... reads more naturally.

There is no guarantee that this function will only pick up on animated vertex colors, though. There could be other IC3fGeomParam or IC4fGeomParam that cause the function to return true. This should be reflected in the name of the function.

515

I don't see how an ICompoundProperty can be nullptr. Maybe you mean to call .valid()?

526

No else after return.

This revision now requires changes to proceed.Oct 1 2020, 2:24 PM

address review comments

address review comments

There still is the "No else after return" comment. That code will fail the next time you run Clang-Tidy.

Sybren A. Stüvel (sybren) requested changes to this revision.Oct 2 2020, 3:23 PM
This revision now requires changes to proceed.Oct 2 2020, 3:23 PM

correct "No else after return", thx bearing with me @Sybren A. Stüvel (sybren)!

Philipp Oeser (lichtwerk) marked 5 inline comments as done.Oct 6 2020, 3:42 PM
This revision is now accepted and ready to land.Oct 7 2020, 10:48 AM