Feature/add ply support #9
Conversation
|
Thank you very much for this PR. I don't know the PLY format very well (I just read wikipedia) so I am trusting that the way the format is read is correct enough. I pointed out some comments though that I think should be addressed. |
|
|
||
| int vertex_count = 0; | ||
| int face_count = 0; | ||
|
|
There was a problem hiding this comment.
My understanding is that all PLY files start with a ply line. I think we should check for that and throw an exit(1) printing "invalid format" to stderr if it is missing.
Also, I read that there are both a binary PLY format and a text PLY format. How can we check that this is indeed a text PLY file?
There was a problem hiding this comment.
It is still pending, but we should check for a line with
format ascii 1.0
in the header.
README.md
Outdated
| @@ -32,6 +32,7 @@ $ ./3d-ascii-viewer --help | |||
|
|
|||
| * [Fox and ShibaInu models](https://opengameart.org/content/fox-and-shiba) made by PixelMannen for the Public Domain (CC0). | |||
| * [Tree models](https://opengameart.org/content/fox-trees-pack) made by Lokesh Mehra (mehrasaur) for the Public Domain (CC0). | |||
| * [PLY models](https://people.sc.fsu.edu/~jburkardt/data/ply/ply.html) made by John Burkardt under the LGPL license. | |||
There was a problem hiding this comment.
The LGPL license is less permissive than the MIT license. So, I think that it is not correct to distribute LGPL-protected content with this MIT licensed repository. Referring to it is fine though.
I am no expert in legal stuff, but my mediocre understanding is that the LGPL, like the GPL, protects with copyleft the content, but is a little more relaxed in the sense that it allows the content to be distributed in for-profit applications as long as some guarantees for the user are protected. The MIT license, on the other hand, allows the software to be used for any purpose, even commercial.
I choose the MIT license since this is a fairly small project and I would like it to be useful for as many people as possible, so I don't think we should change the license. Also, there is a contributor interested in FreeBSD portability (the BSD license is quite similar to the MIT license, also very permissive). See #1 .
There was a problem hiding this comment.
I removed the models from the commits for now. It would be nice if we could find some CC0 (public domain) PLY models, or export some CC0 models as PLY and include them in the repo.
| vec.x = f1; | ||
| vec.y = f2; | ||
| vec.z = f3; |
There was a problem hiding this comment.
I noticed that the models appear rotated so probably this is not the correct ordering for the axis. Perhaps vec.y should be f3 and vec.z should be f2 ?
There was a problem hiding this comment.
Actually, the ordering of the axes came inconsistent between the models PLY models. So the teapot.ply and the airplane.py had the wrong orientation.
Perhaps we should stick to the axes ordering used by blender when exporting .PLY files.
Co-authored-by: Francisco Casas <[email protected]>
Co-authored-by: Francisco Casas <[email protected]>
autopawn
force-pushed
the
feature/add_ply_support
branch
from
6c43989 to
8661152
Compare
|
I rebased the PR on top of current master an addressed some of the comments. Some are still pending. |
This PR adds PLY support.