Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 64 additions & 2 deletions src/draco/io/ply_decoder.cc
Original file line number Diff line number Diff line change
@@ -70,8 +70,11 @@ Status PlyDecoder::DecodeInternal() {
PlyReader ply_reader;
DRACO_RETURN_IF_ERROR(ply_reader.Read(buffer()));
// First, decode the connectivity data.
if (out_mesh_)
DRACO_RETURN_IF_ERROR(DecodeFaceData(ply_reader.GetElementByName("face")));
if (out_mesh_) {
const PlyElement *face_element = ply_reader.GetElementByName("face");
DRACO_RETURN_IF_ERROR(DecodeFaceData(face_element));
DRACO_RETURN_IF_ERROR(DecodeTexCoordData(face_element));
}
// Decode all attributes.
DRACO_RETURN_IF_ERROR(
DecodeVertexData(ply_reader.GetElementByName("vertex")));
@@ -136,6 +139,65 @@ Status PlyDecoder::DecodeFaceData(const PlyElement *face_element) {
return OkStatus();
}

Status PlyDecoder::DecodeTexCoordData(const PlyElement *face_element) {
if (face_element == nullptr) {
return Status(Status::INVALID_PARAMETER, "face_element is null");
}
const PlyProperty *texture_coordinates =
face_element->GetPropertyByName("texcoord");
if (texture_coordinates == nullptr || !texture_coordinates->is_list()) {
return OkStatus();
}

// Allocate attribute for texture coordinates.
// There is one pair of texture coordinates per triangle corner.
const int num_corners = out_mesh_->num_faces() * 3;


std::unique_ptr<PointAttribute> attr(new PointAttribute());
attr->Init(GeometryAttribute::TEX_COORD, 2, DT_FLOAT32, false, num_corners);

PlyPropertyReader<float> uv_reader(texture_coordinates);
AttributeValueIndex corner_index(0);

auto pushTexcoordPair = [uv_reader, &corner_index, &attr]
(uint64_t offset, uint64_t index) -> void {
float uv_value[2];
uv_value[0] = uv_reader.ReadValue(static_cast<int>(offset + index * 2));
uv_value[1] = uv_reader.ReadValue(static_cast<int>(offset + index * 2 + 1));
attr->SetAttributeValue(corner_index, uv_value);
corner_index++;
};

const int64_t num_polygons = face_element->num_entries();
for (int i = 0; i < num_polygons; ++i) {
const int64_t uv_list_offset = texture_coordinates->GetListEntryOffset(i);
const int64_t uv_list_size = texture_coordinates->GetListEntryNumValues(i);
if (uv_list_size < 6 || uv_list_size % 2 != 0) {
continue; // Need at least three pairs of UV coordinates per polygon.
}

// Triangulate polygon assuming the polygon is convex.
const int64_t num_triangles = uv_list_size / 2 - 2;

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}
}

IndexTypeVector<CornerIndex, AttributeValueIndex> corner_map(num_corners);
for (CornerIndex ci(0); ci < num_corners; ++ci) {
corner_map[ci] = AttributeValueIndex(ci.value());
}
// I don't think it works to have this as the only point-mapped attribute
// while the rest of the attributes are identity-mapped.
out_mesh_->AddAttributeWithConnectivity(std::move(attr), corner_map);
return OkStatus();
}

template <typename DataTypeT>
bool PlyDecoder::ReadPropertiesToAttribute(
const std::vector<const PlyProperty *> &properties,
2 changes: 2 additions & 0 deletions src/draco/io/ply_decoder.h
Original file line number Diff line number Diff line change
@@ -49,6 +49,8 @@ class PlyDecoder {

private:
Status DecodeFaceData(const PlyElement *face_element);
Status DecodeTexCoordData(const PlyElement *face_element);

Status DecodeVertexData(const PlyElement *vertex_element);

template <typename DataTypeT>
15 changes: 15 additions & 0 deletions src/draco/io/ply_decoder_test.cc
Original file line number Diff line number Diff line change
@@ -77,6 +77,21 @@ TEST_F(PlyDecoderTest, TestPlyNormals) {
ASSERT_EQ(att->size(), 6); // 6 unique normal values.
}

TEST_F(PlyDecoderTest, TestPlyTexCoords) {
const std::string file_name = "cube_att.ply";
std::unique_ptr<Mesh> mesh;
test_decoding(file_name, 12, 3 * 8, &mesh);
ASSERT_NE(mesh, nullptr);
const int att_id = mesh->GetNamedAttributeId(GeometryAttribute::TEX_COORD);
ASSERT_GE(att_id, 0);
const PointAttribute *const att = mesh->attribute(att_id);
ASSERT_EQ(att->size(), 4); // 4 unique texture coordinate values.
float vertex_0_tex_coord[2];
att->GetValue(AttributeValueIndex(0), vertex_0_tex_coord);
ASSERT_EQ(vertex_0_tex_coord[0], 1);
ASSERT_EQ(vertex_0_tex_coord[1], 1);
}

TEST_F(PlyDecoderTest, TestPlyDecodingAll) {
// test if we can read all ply that are currently in test folder.
test_decoding("bun_zipper.ply");
2 changes: 2 additions & 0 deletions src/draco/mesh/mesh.cc
Original file line number Diff line number Diff line change
@@ -68,6 +68,7 @@ void Mesh::Copy(const Mesh &src) {
// Copy structural metadata.
structural_metadata_.Copy(src.structural_metadata_);
}
#endif

namespace {
// A helper struct that augments a point index with an attribute value index.
@@ -182,6 +183,7 @@ int32_t Mesh::AddAttributeWithConnectivity(
return PointCloud::AddAttribute(std::move(att));
}

#ifdef DRACO_TRANSCODER_SUPPORTED
int32_t Mesh::AddPerVertexAttribute(std::unique_ptr<PointAttribute> att) {
const PointAttribute *const pos_att =
GetNamedAttribute(GeometryAttribute::POSITION);
2 changes: 1 addition & 1 deletion src/draco/mesh/mesh.h
Original file line number Diff line number Diff line change
@@ -96,7 +96,6 @@ class Mesh : public PointCloud {
}
}

#ifdef DRACO_TRANSCODER_SUPPORTED
// Adds a point attribute |att| to the mesh and returns the index of the
// newly inserted attribute. Attribute connectivity data is specified in
// |corner_to_value| array that contains mapping between face corners and
@@ -108,6 +107,7 @@ class Mesh : public PointCloud {
std::unique_ptr<PointAttribute> att,
const IndexTypeVector<CornerIndex, AttributeValueIndex> &corner_to_value);

#ifdef DRACO_TRANSCODER_SUPPORTED
// Adds a point attribute |att| to the mesh and returns the index of the
// newly inserted attribute. The inserted attribute must have the same
// connectivity as the position attribute of the mesh (that is, the attribute