Skip to content

Allow use of mesh_demo_v2 with PLY files lacking vertex colors #2578

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nfelt
Copy link
Contributor

@nfelt nfelt commented Aug 19, 2019

This makes it somewhat easier to run the demo, since at least in my quick pass over the internet, a decent number of PLY files don't contain vertex colors (like the top result for "teapot ply" on google).

It seems reasonable to support since the color parameter to the summary op itself is optional.

I only added support in the v2 demo since the v1 demo uses graph mode and making the color optional there would be a little more work (and for my purposes, I just want at least one script I can easily use to generate sample summary data).

PTAL @podlipensky

@wchargin
Copy link
Contributor

For what it’s worth, we do have a sample PLY mesh with color data in the
repository for testing purposes:
https://github.com/tensorflow/tensorboard/blob/master/tensorboard/plugins/mesh/test_data/icosphere.ply

(The mesh data is a little wonky (dupe verts) because I screwed it up
when creating the file, and I’ve been meaning to replace it with a
non-borked version that also shows orientation, but haven’t gotten
around to it. It works just fine, though.)

@nfelt
Copy link
Contributor Author

nfelt commented Aug 19, 2019

Thanks for pointing that out, I totally missed that 🤦‍♂️ It still seems useful to me to allow non-colored meshes here, but I am happy to change the demo so that the default behavior uses that in-repo file if @podlipensky thinks that's reasonable.

@nfelt nfelt mentioned this pull request May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants