Support reading uv and uv map for ply format if texture_uv exists in ply file #1100
Conversation
|
Hi @YangHai-1218! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
1 similar comment
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
Thanks. This looks like the right logic.
Do you have an example of data in this format which we can use as a test for this new functionality? (It would need to be liberally licensed.) Otherwise, can you create a small synthetic example?
In particular: The UV coordinates are sent directly from the PLY to TexturesUV without changing their range or direction. Without a real example, I cannot tell whether that is correct.
| @@ -1073,7 +1073,7 @@ def join_batch(self, textures: List["TexturesUV"]) -> "TexturesUV": | |||
| faces_uvs_list += self.faces_uvs_list() | |||
| verts_uvs_list += self.verts_uvs_list() | |||
| maps_list += self.maps_list() | |||
| num_faces_per_mesh = self._num_faces_per_mesh | |||
| num_faces_per_mesh = self._num_faces_per_mesh.copy() | |||
There was a problem hiding this comment.
This also looks like a separate significant bug you've fixed, best in its own PR. Thanks again. Probably the test change would be an extra assertion in an existing test.
(Or let us know and we can sort it out.
There was a problem hiding this comment.
In fact, I think TexturesAtlas has exactly the same bug. They should be fixed at the same time.
There was a problem hiding this comment.
Yes, this is a significant bug which was found when I was using Pytorch3D. Specifically, for meshes with TextureUV, calling function join_meshes_as_batch will cause the origin mesh textureUV's _num_faces_per_mesh changed, due to the list reference attribute. Especially when calling join_meshes_per_mesh multiple times, this will cause OOM finally, because the original mesh textureUV's _num_faces_per_mesh list has became too long. I have not noticed the TextureAltas has the same bug because I have not used it. This is just a small change, so I committed it together. Can you please sort it out if needed?
There was a problem hiding this comment.
Yes. Let me sort it out. Best to remove this change from this PR still.
| @@ -110,7 +110,9 @@ def __init__(self, cameras=None, raster_settings=None) -> None: | |||
|
|
|||
| def to(self, device): | |||
| # Manually move to device cameras as it is not a subclass of nn.Module | |||
| self.cameras = self.cameras.to(device) | |||
| cameras = self.cameras | |||
| if cameras is not None: | |||
There was a problem hiding this comment.
This looks like a significant bug you are fixing, which is separate from this change. Thank you. It would be good to put it in its own PR which would include a test for this problem.
(Or let us know and we can do it internally.)
This is a test mesh, which contains a ply file and its texture file. It is licensed. You can test the code with it. |
|
What do you mean by "it is licensed"? We need to use an existing example with a known permissive license, or to create a new example. (It can then be added to tests/data and a test validating what it looks like against an image can be created. This should happen in this PR.) |
|
Its license is MIT. Is this enough? If not, I will create an example to do testing. |
Summary: As reported in #1100, _num_faces_per_mesh was changing in the source mesh in join_batch. This affects both TexturesUV and TexturesAtlas Reviewed By: nikhilaravi Differential Revision: D34643675 fbshipit-source-id: d67bdaca7278f18a76cfb15ba59d0ea85575bd36
Summary: As reported in #1100, a rasterizer couldn't be moved if it was missing the optional cameras member. Fix that. This matters because the renderer.to calls rasterizer.to, so this to() could be called even by a user who never sets a cameras member. Reviewed By: nikhilaravi Differential Revision: D34643841 fbshipit-source-id: 7e26e32e8bc585eb1ee533052754a7b59bc7467a
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR was closed because it has been stalled for 10 days with no activity. |
|
@bottler has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@bottler has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Thanks! |
When the ply format looks as follows:
MeshPlyFormatclass will read uv from the ply file and read the uv map as commented as TextureFile.