Alembic: support reading per-vertex UV sets
ClosedPublic
Actions

Authored by Kévin Dietrich (kevindietrich) on Jun 11 2021, 6:59 PM.

Details

Reviewers

Sybren A. Stüvel (sybren)

Commits

rB3385c04598f2: Alembic: support reading per-vertex UV sets

Summary

In Alembic, UVs can be defined with different scopes. The most common one
is "face varying" scope, which maps to Blender's polygon loops, where a value
is stored for each vertex of each polygon. This makes sense as a given vertex
might be on a UV seam and thus have multiple possible UV coordinates.

However, it is also possible to define UVs using a "vertex" scope, meaning that
we will have a single UV value for each vertex. This can happen when the mesh
is split according to the UV islands, in which case it makes sense to only store
one value per vertex, as vertices are guaranteed to only have a single possible
UV coordinate.

Currently, the Alembic importer only supports the former case, this patch
implements the latter case. The implementation is pretty straighforward,
we mainly have to properly check whether the size of the UVs indices given
by the sample matches the size of the attribute domain (loops vs. vertices).
UVs are then assigned to the loop UVs based on the index of the vertex pointed
to by the loop.

Here is a test file (.blend and .abc), the UV map is not imported in Blender 2.93:

test_abc_uvs_per_vertex.blend827 KBDownload

test_abc_uvs_per_vertex.abc5 KBDownload

Diff Detail

Repository

rB Blender

Branch

abc_vertex_uvs_D11584 (branched from master)

Build Status

Buildable 15164
Build 15164: arc lint + arc unit

Event Timeline

Kévin Dietrich (kevindietrich) requested review of this revision.Jun 11 2021, 6:59 PM

Kévin Dietrich (kevindietrich) created this revision.

Harbormaster completed remote builds in B15140: Diff 38220.Jun 11 2021, 6:59 PM

Kévin Dietrich (kevindietrich) edited the summary of this revision. (Show Details)Jun 11 2021, 7:02 PM

Robert Rioux (riouxr) awarded a token.Jun 11 2021, 10:18 PM

The functionality is nice to have, but there are some potential improvements to the implementation.

source/blender/io/alembic/intern/abc_customdata.cc
336–364	There is a lot of duplication between the two conditional blocks, which should be resolved. The duplication could make sense from a performance point of view, but in such a case it should be profiled and a decision made based on actual data. Without that, I would prefer clean code.
source/blender/io/alembic/intern/abc_reader_mesh.cc
196–197	This duplicates the same logic from `read_uvs()`. It's better to relay such decisions to a separate function, and use that in both cases. This function could then sit next to `is_valid_uv_scope()`.
229–240	The only difference between these two blocks is `uv_index = (uvs_indices)[loop_index];` vs. `uv_index = (uvs_indices)[loop.v];`. I'm sure this can be deduplicated.
source/blender/io/alembic/intern/abc_util.cc
233–235 ↗	(On Diff #38220)	I think all parameters can be `const`
240–246 ↗	(On Diff #38220)	This is tightly coupled to the `read_uvs()` and `read_mpolys()` functions. However, these are in different files, so this relation is not clear.

This revision now requires changes to proceed.Jun 14 2021, 12:00 PM

Deduplicated logic in loops.

I am not sure what you meant by the relation is not clear between
is_valid_uv_scope, read_uvs, and read_mcol. However, I revised
the function signature and moved is_valid_uv_scope in abc_customdata.h.

Added get_uv_scope to determine in which scope the UVs are depending on
whether the indices size matches that of the loops or the vertices.

Harbormaster completed remote builds in B15164: Diff 38265.Jun 14 2021, 1:35 PM

In D11584#297240, @Kévin Dietrich (kevindietrich) wrote:

I am not sure what you meant by the relation is not clear between is_valid_uv_scope, read_uvs, and read_mcol.

I mean that these functions are defined in different files, and there is no comment or code design that makes this relation explicit.

However, I revised the function signature and moved is_valid_uv_scope in abc_customdata.h.

That's already an improvement. I do wonder, however, whether we actually need both. Now that I see them side-by-side, they are very similar. Would it be possible to merge the two, so that get_uv_scope() becomes safer to use (because it also includes a check on the geometry scope declared in the Alembic file), and is_valid_uv_scope() can be reduced to get_uv_scope() != ABC_UV_SCOPE_NONE.