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

Fix PLY save bug: vertex_index -> vertex_indices. #579

Merged
merged 2 commits into from
Mar 20, 2014
Merged

Fix PLY save bug: vertex_index -> vertex_indices. #579

merged 2 commits into from
Mar 20, 2014

Conversation

yangyanli
Copy link
Contributor

PLYReader does not take the *.ply files generated by PLYWriter due to the mismatch between vertex_index and vertex_indices. And it seems to be the fault of PLYWriter, so change vertex_index to vertex_indices.

@taketwo
Copy link
Member

taketwo commented Mar 19, 2014

Could you please also check why this test passes?

@nizar-sallem
Copy link
Contributor

@taketwo +1 it is a bug on my side.

@yangyanli
Copy link
Contributor Author

test_ply_mesh_io.cpp does save with savePLYFile, and read with loadPolygonFilePLY. loadPolygonFilePLY uses ply reader from vtk, which may take vertex_index, but loadPLYFile doesn't.

It will be better if loadPLYFile can be added to the test.

@taketwo
Copy link
Member

taketwo commented Mar 19, 2014

@yangyanli Could you add a simple regression test for the bug?

@taketwo
Copy link
Member

taketwo commented Mar 19, 2014

But wait, how come that you changed the format that savePLYFile uses, but loadPolygonFilePLY is still able to read it (because the test still passes).

@yangyanli
Copy link
Contributor Author

loadPolygonFilePLY is more tolerant than loadPLYFile. loadPLYFile only takes vertex_indices, loadPolygonFilePLY takes both.

I do not have test build on my machines, and have no experience on the regression test, so it might take a while for me to do that.

@taketwo
Copy link
Member

taketwo commented Mar 19, 2014

@yangyanli Well "regression test" is just a fancy name. Basically we just need to make sure that there is a test which fails with old code and succeeds with new code. In this particular case it is sufficient to add loadPLYFile (along with loadPolygonFilePLY) to the existing test.

Also, you do not need to build all tests. Simply enable them (e.g. using ccmake) and then type make test_ply_mesh_io.

@taketwo
Copy link
Member

taketwo commented Mar 20, 2014

Great! Merging this.

taketwo added a commit that referenced this pull request Mar 20, 2014
Fix PLY save bug: vertex_index -> vertex_indices.
@taketwo taketwo merged commit a850840 into PointCloudLibrary:master Mar 20, 2014
@yangyanli yangyanli deleted the ply_fix branch March 20, 2014 15:47
@yangyanli yangyanli restored the ply_fix branch March 30, 2014 04:35
@yangyanli yangyanli deleted the ply_fix branch March 30, 2014 04:38
@yangyanli yangyanli restored the ply_fix branch April 24, 2014 16:27
@yangyanli yangyanli deleted the ply_fix branch April 24, 2014 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants