Add support for per-face texture coordinates in the PLY decoder.

[kbongort]

This PR adds support to the PlyDecoder for decoding texture coordinates from the texcoord property of the face element.

PLY files can support a single vertex with different texture coordinates per face. Since Draco expects texture coordinates to be stored per-vertex, this implementation chooses the last set of texture coordinates found for each vertex. For many users this should not be an issue. Where it is a problem, it's no worse than not having texture coordinate support at all.

[ondys]

Draco doesn't really expect texture coordinates to be stored per-vertex. In fact all non-position attributes are usually stored per-corner (so each corner of a single vertex can have a different attribute value). Given that, we would prefer to have a solution that would decode the texture coordinates properly. The solution in this PR would probably lead to even more confusion and hard-to-track bugs down the line.

[kbongort]

Draco doesn't really expect texture coordinates to be stored per-vertex. In fact all non-position attributes are usually stored per-corner (so each corner of a single vertex can have a different attribute value). Given that, we would prefer to have a solution that would decode the texture coordinates properly. The solution in this PR would probably lead to even more confusion and hard-to-track bugs down the line.

Am I correct in thinking that would require a more substantial rewrite of the PlyDecoder to use point mapping rather than identity mapping, or is there a shortcut I'm missing?

[ondys]

Draco doesn't really expect texture coordinates to be stored per-vertex. In fact all non-position attributes are usually stored per-corner (so each corner of a single vertex can have a different attribute value). Given that, we would prefer to have a solution that would decode the texture coordinates properly. The solution in this PR would probably lead to even more confusion and hard-to-track bugs down the line.

Am I correct in thinking that would require a more substantial rewrite of the PlyDecoder to use point mapping rather than identity mapping, or is there a shortcut I'm missing?

Not necessarily. The easiest way would be to use Mesh::AddAttributeWithConnectivity() method. It would allow you to specify mapping between corners and attribute values and it would automatically update the point mapping. That method is currently defined within #ifdef DRACO_TRANSCODER_SUPPORTED but it can be probably moved out if needed.

[kbongort]

Not necessarily. The easiest way would be to use Mesh::AddAttributeWithConnectivity() method. It would allow you to specify mapping between corners and attribute values and it would automatically update the point mapping. That method is currently defined within #ifdef DRACO_TRANSCODER_SUPPORTED but it can be probably moved out if needed.

Thanks for your patience. I tried using Mesh::AddAttributeWithConnectivity(), but it doesn't work, either failing at IsAddressValid() in ConvertTypedValue() or crashing later. I suspect this is because the other attributes used by the PLY decoder are identity-mapped, and using a mix of identity-mapped and point-mapped attributes doesn't work. Does that make sense?

I've pushed a commit here with my (non-working) code, in case you are able to spot something I'm missing.

[ondys]

I don't think this would add one value for each corner, not to mention the corner index seems to be wrong.

Basically you would have to add value for each corner where the corner index is (3 * triangle_index + c) where c={0, 1, 2}

[kbongort]

Good catch, fixed. This moves through vertices in the same order as in DecodeFaceData. When triangulating a polygon with more than 3 sides, the first vertex is repeated in each triangle.

[ondys]

I understand that the first vertex is repeated but even so, each triangle of the polygon will have a distinct corner attached to the "first" vertex. E.g. if you have a quad there will be two corners attached to it. In the current implementation only the corner from the first triangle is going to be processed which is wrong because it messes up the way how Draco maps corners to triangles + it leaves some corners with uninitialized values. Again, as an example for a quad, we would expect to have two triangles with corners: (0, 1, 2), (3, 4, 5) where they would be mapped to values (0, 1, 2), (0, 2, 3).

[kbongort]

Just to make sure, are you looking at the change from the latest commit? eee2335

for (int64_t ti = 0; ti < num_triangles; ++ti) {
  pushTexcoordPair(uv_list_offset, 0);
  for (int64_t c = 1; c < 3; ++c) {
    pushTexcoordPair(uv_list_offset, ti + c);
  }
}

For a quad with corners (0, 1, 2, 3), this should push 6 pairs of UVs in the order (0, 1, 2, 0, 2, 3). Is that not correct?

[ondys]

Sorry about that. Not sure what I was looking at, you are right.

One other potential issue I can see is that, if I understand it correctly, the DecodeTexCoordData() is called before we decode any vertex data. I think this may cause some issues because AddAttributeWithConnectivity assumes that we have some valid points in the mesh already. Can you try to move this decoding after DecodeVertexData() to see if it helps?

[ondys]

the number of points should be recomputed automatically when the function is called. I think this may actually cause some issues in the subsequent function.

[kbongort]

I've made this change, but it crashes because the is_point_used vector is initialized with its size set to num_points(), which is 0 if we don't set the number here. That results in out-of-bounds access.

I've tried setting the number of points here to the number of corners, and the number of vertices. Nothing works; the program either crashes or fails non-deterministically at various points.