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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e93d8b5
Add support for per-face texture coordinates in the PLY decoder.
kbongort Nov 12, 2023
6e4ea15
80 chars
kbongort Nov 13, 2023
d5b3fa1
Attempt to use AddAttributeWithConnectivity.
kbongort Dec 16, 2023
eee2335
Update per comments.
kbongort Jan 13, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")));
Expand Down Expand Up @@ -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;
pushTexcoordPair(uv_list_offset, 0);
for (int64_t ti = 0; ti < num_triangles; ++ti) {
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_->set_num_points(num_corners);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

@kbongort kbongort Jan 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

out_mesh_->AddAttributeWithConnectivity(std::move(attr), corner_map);
return OkStatus();
}

template <typename DataTypeT>
bool PlyDecoder::ReadPropertiesToAttribute(
const std::vector<const PlyProperty *> &properties,
Expand Down
2 changes: 2 additions & 0 deletions src/draco/io/ply_decoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
15 changes: 15 additions & 0 deletions src/draco/io/ply_decoder_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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], 0);
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");
Expand Down
2 changes: 2 additions & 0 deletions src/draco/mesh/mesh.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/draco/mesh/mesh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down