Skip to content

Replace .ply reader/writer with tinyply library#1422

Merged
alecjacobson merged 2 commits intolibigl:masterfrom
vfonov:tinyply_reader_writer
Apr 11, 2020
Merged

Replace .ply reader/writer with tinyply library#1422
alecjacobson merged 2 commits intolibigl:masterfrom
vfonov:tinyply_reader_writer

Conversation

@vfonov
Copy link
Copy Markdown
Contributor

@vfonov vfonov commented Feb 14, 2020

Using tinyply library for reading and writing .ply files:

based on #1235, allows reading and writing additional per-vertex, per-face and per-edge properties, using tinyply version 2.3.2.

[Describe your changes and what you've already done to test it.]

Check all that apply (change to [x])

  • All changes meet libigl style-guidelines.
  • Adds new .cpp file.
  • Adds corresponding unit test.
  • Adds corresponding python binding.
  • This is a minor change.

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Feb 14, 2020

Thanks! Did you also update the libigl-test-repo and point this PR to the update SHA1 so it can be tested on Travis before we merge it?

@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Feb 14, 2020

ok, I forgot about that

@vfonov vfonov force-pushed the tinyply_reader_writer branch from 4a33eb3 to dc1a2d3 Compare February 14, 2020 18:58
@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Feb 14, 2020

Thanks. How about mesh_error.ply though, do you have it?

@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Feb 14, 2020

it's supposed to be missing : https://github.com/vfonov/libigl/blob/tinyply_reader_writer/tests/include/igl/readPLY.cpp#L69

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Feb 14, 2020

Ahah my bad. I'll take a look at the PR this weekend then, hopefully we can merge it.

@vfonov vfonov force-pushed the tinyply_reader_writer branch 2 times, most recently from aba726c to 32e8fb1 Compare February 20, 2020 21:28
@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Feb 20, 2020

I made more changes and also updated SHA for test data.

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Feb 24, 2020

Hmm seems like it's still not building on Travis/AppVeyor...

@vfonov vfonov force-pushed the tinyply_reader_writer branch from 32e8fb1 to 518a000 Compare February 24, 2020 20:47
@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Feb 24, 2020 •
edited
Loading

ok, I think I got why there were errors with template instantiation ... or maybe not

@vfonov vfonov force-pushed the tinyply_reader_writer branch from 518a000 to 9b37c19 Compare February 24, 2020 23:25
@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Feb 24, 2020

another attempt...

@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Feb 25, 2020

At least it builds with gcc-7, looks like something is wrong with the build setup for macosX though

@vfonov vfonov force-pushed the tinyply_reader_writer branch 2 times, most recently from 5373569 to d12173c Compare February 25, 2020 18:28
@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Feb 25, 2020 •
edited
Loading

so, appveyor is timing out now: "Build execution time has reached the maximum allowed time for your plan (60 minutes)."

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Feb 25, 2020

Yeah the timeout issue on AppVeyor is a known issue. For the build on macOS you should rebase your changes onto our dev branch, which contains fixes to the .travis.yml for macOS.

@vfonov vfonov force-pushed the tinyply_reader_writer branch from d12173c to 8458489 Compare February 25, 2020 20:55
@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Feb 25, 2020

looks like rebasing didn't help

@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Feb 25, 2020

maybe you need to add command to explicitly install ccache, as per https://docs.travis-ci.com/user/caching/#ccache-on-macos ?

@vfonov vfonov force-pushed the tinyply_reader_writer branch from 8458489 to 3be9544 Compare February 25, 2020 22:24
@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Feb 25, 2020

No ccache should be available thanks to this line. But honestly I'm not super inclined to debug build issues on Travis, since I'm waiting on #1389 to get merged and we switch to GitHub Actions for CI builds. Most likely I suspect this is not gonna happen till March 20 where we'll have our next libigl hackaton planned.

@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Feb 26, 2020

so, installing ccache with brew install ccache seem to have worked - the only thing crashing now is CGAL on windows, but I think it's not related.

@vfonov vfonov force-pushed the tinyply_reader_writer branch 3 times, most recently from b784952 to 1391a68 Compare February 28, 2020 22:46
@BruegelN BruegelN mentioned this pull request Mar 10, 2020
Merged
5 tasks
@alecjacobson
Copy link
Copy Markdown
Contributor

alecjacobson commented Mar 11, 2020

The current ply reader appears to fail on bun_zipper.ply (only loads two coordinates instead of all three). Could you try this model with this PR and see if it loads correctly? (then I don't have to debug the old reader). http://graphics.stanford.edu/pub/3Dscanrep/bunny.tar.gz

@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Mar 11, 2020

Seem to work: showing bunny/reconstruction/bun_zipper.ply with "confidence" property as per-vertex data
image

@vfonov vfonov force-pushed the tinyply_reader_writer branch 2 times, most recently from 9a4c09d to de473ed Compare March 12, 2020 14:24
@alecjacobson
Copy link
Copy Markdown
Contributor

alecjacobson commented Mar 13, 2020

the current readPLY seems to fail consistently on these plys https://gfx.cs.princeton.edu/proj/sugcon/models/

Does this PR succeed on those?

@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Mar 13, 2020 •
edited
Loading

I made a small test program : check_ply.cpp check_ply.cpp.txt and tested it on the PLY files from the location. Here is the output. It looks like all these models are using "tristrips" property instead of "faces" , so loader doesn't load faces matrix (it's 0x0):


PLY:armadillo.ply Success:	V: 172974x3	F: 0x0	D: 0x0	
PLY:brain2.ply Success:	V: 294012x3	F: 0x0	D: 0x0	
PLY:brain.ply Success:	V: 18844x3	F: 0x0	D: 0x0	
PLY:bunny2.ply Success:	V: 72027x3	F: 0x0	D: 0x0	
PLY:bunny.ply Success:	V: 34834x3	F: 0x0	D: 0x0	
PLY:cow2.ply Success:	V: 46433x3	F: 0x0	D: 0x0	
PLY:cow.ply Success:	V: 2903x3	F: 0x0	D: 0x0	
PLY:elephant2.ply Success:	V: 78792x3	F: 0x0	D: 0x0	
PLY:elephant.ply Success:	V: 19753x3	F: 0x0	D: 0x0	
PLY:golfball.ply Success:	V: 122882x3	F: 0x0	D: 0x0	
PLY:heptoroid.ply Success:	V: 286678x3	F: 0x0	D: 0x0	
PLY:hippo.ply Success:	V: 23105x3	F: 0x0	D: 0x0	
PLY:horse2.ply Success:	V: 48484x3	F: 0x0	D: 0x0	
PLY:horse.ply Success:	V: 48484x3	F: 0x0	D: 0x0	
PLY:igea.ply Success:	V: 134345x3	F: 0x0	D: 0x0	
PLY:lion2.ply Success:	V: 183408x3	F: 0x0	D: 183408x3	diffuse_red,diffuse_green,diffuse_blue,
PLY:lion.ply Success:	V: 183408x3	F: 0x0	D: 183408x3	diffuse_red,diffuse_green,diffuse_blue,
PLY:lucy.ply Success:	V: 262909x3	F: 0x0	D: 0x0	
PLY:maxplanck.ply Success:	V: 49132x3	F: 0x0	D: 0x0	
PLY:pear.ply Success:	V: 10759x3	F: 0x0	D: 0x0	
PLY:santa2.ply Success:	V: 75781x3	F: 0x0	D: 75781x3	diffuse_red,diffuse_green,diffuse_blue,
PLY:santa.ply Success:	V: 75781x3	F: 0x0	D: 75781x3	diffuse_red,diffuse_green,diffuse_blue,
PLY:torus.ply Success:	V: 4800x3	F: 0x0	D: 0x0

@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Mar 13, 2020

ok, I managed to treat this case, will push another commit soon

@vfonov vfonov force-pushed the tinyply_reader_writer branch from de473ed to d61a679 Compare March 13, 2020 18:24
@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Mar 13, 2020

The latest version can read ply files with tristrips instead of faces.
I modified my check script, and run on all the ply files from the princeton library.
check_ply.cpp.txt
armadillo ply
brain ply
brain2 ply
bunny ply
bunny2 ply
cow ply
cow2 ply
elephant ply
elephant2 ply
golfball ply
heptoroid ply
hippo ply
horse ply
horse2 ply
igea ply
lion ply
lion2 ply
lucy ply
maxplanck ply
pear ply
santa ply
santa2 ply
torus ply

@vfonov
Replace .ply reader/writer with tinyply library, supersedes libigl#1235
52c6f14 
Updated SHA for libigl-tests-data
removed obsolete ply.h
updated tinyply to compile with and without LIBIGL_USE_STATIC_LIBRARY
fixed template instantiation signatures to work with gcc
fixed integer classes for visual studio
fixed igl::tynyply_type redefinition
added command to install ccache in .travis.yml
added test for writePLY
changed asserts inside writePLY code
fixed some bugs in writePLY code
made PLY reader/writer work with quad meshes
made PLY reader/writer work with tristrips
@vfonov vfonov force-pushed the tinyply_reader_writer branch from d61a679 to 52c6f14 Compare March 24, 2020 22:15
@vfonov
Copy link
Copy Markdown
Contributor Author

vfonov commented Mar 25, 2020

So, is there some concerns about including these changes into the dev branch?

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Mar 25, 2020

@alecjacobson is probably busy with the SIGGRAPH committee until the end of the week. Once he gives the green light we can merge this.

@teseoch teseoch changed the base branch from dev to master April 11, 2020 16:34
@jdumas jdumas requested a review from alecjacobson April 11, 2020 17:34
@alecjacobson alecjacobson merged commit 10dc1fd into libigl:master Apr 11, 2020
@jdumas jdumas mentioned this pull request Apr 11, 2020
Closed
5 tasks
@vfonov vfonov deleted the tinyply_reader_writer branch April 14, 2020 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alecjacobson alecjacobson alecjacobson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vfonov @jdumas @alecjacobson