Various improvements #5

bchretien · 2017-01-30T10:20:43Z

tinyply now supports variable-length lists (fix Variable-length lists are not supported #3).
Add CMake support (minimum version = 3.0), with CMake module generation and pkg-config support for Linux.
Fix a bug when elements are skipped entirely (e.g. only extract face information).
Add some basic tests for reading/writing. Checks still need to be added, for now they just process ASCII/binary data and print some output.
Add Travis CI support (for Trusty). You might want to enable it on the project. See the result for my fork.
Add new is_binary() method that allows user to know whether the PLY being read is in ASCII or binary format.
Remove trailing whitespaces when generating ASCII PLY files.

- Build library and example. - Generate tinyply.pc for pkg-config support. - Generate CMake package configuration files. - Install relevant files.

bchretien · 2017-01-30T12:33:57Z

We could also remove the tinyply.xcodeproj and tinyply.vcxproj directories since CMake can generate solutions for VS and Xcode (untested).

bchretien · 2017-01-30T13:05:53Z

I also found a possible source of error. PlyFile can take 2 distinct std::istreams when reading a file: one in the constructor to read the header, and another one for the actual data. However, if the user creates a new std::istream for the second one, since the second stream might not be pointing to the correct position in the file (after the header section), this will lead to an error.

ddiakopoulos · 2017-02-16T17:38:32Z

Just FYI: still on my list of things to review. Just a busy time of the year right now!

maruncz · 2017-02-16T21:22:46Z

hi i tried your patch and it has bug, when i try to load mesh i got memcheck error at invalid read of size 8, tinyply.h:163

i found that error, if you don't specify listCount in request_properties_from_element when requesting elements it will crash

but i thought this parameter is optional, because i cannot specify it because i don't know file format

bchretien · 2017-02-20T09:50:55Z

@maruncz thanks for the feedback, I'll fix that ASAP. Could you provide an example PLY and the few property registration lines you used to get that error?

but i thought this parameter is optional, because i cannot specify it because i don't know file format

You mean you don't know the number of elements for that specific property?

maruncz · 2017-02-20T09:56:41Z

@bchretien https://github.com/maruncz/VR-pointcloud-viewer/blob/master/src/readers/plyreader.cpp
i added listCount = 3 into code

https://marun.uk.to/owncloud/s/SgXer13EtxGjEsC here is testing file

bchretien · 2017-02-20T14:39:26Z

The rationale behind the support of variable-length lists is the following one: if we take the faces for example, they could be stored as triangles (3 indices) or quads (4 indices), or even both in the same file (cf. #3). Thus, we need to be able to distinguish all those cases: either the length of the list is known and fixed (e.g. only triangles are supported, so listCount = 3), or it is unknown/variable. In that case, we need to set listCount = 0 (or any negative value), and use a std::vector<std::vector<T>>.

In your reader, you only support faces with triangles, correct?

maruncz · 2017-02-20T15:07:00Z

yes i support only triangles

but should'd be default listCount value 0 now?

bchretien · 2017-02-20T16:31:12Z

I don't think it should since we don't want to break the existing behavior. Also:

If a std::vector<...> is provided, we can keep 1 as the default value as it implies that a single value is used for that property (so not a list). This is the common use case.
If a std::vector<...> is provided for a fixed-size list, listCount should be > 1. We expect the user to provide that information since the format does not explicitly support fixed-size lists (they just happen to be variable-length lists that all have the same length).
If a std::vector<std::vector<...>> is provided, listCount should be ignored (I just fixed that in 63a39e6).

Now, there might be some checks/exceptions missing though.

maruncz · 2017-10-02T15:07:45Z

hi do you still plan to merge this request?

ddiakopoulos · 2017-10-02T18:13:28Z

I don't think so. I just finished a re-write of the API and some of the parsing functionality (i'll put it on a 2.0 branch soon) because I wasn't happy with the way the variable-length list support was introduced here... I made some questionable design decisions early on and this made some of these changes by @bchretien more complex than I would have liked.

I do like the CI + cmake + testing + other bugfixes, so I'll manually cherry pick those eventually.

maruncz · 2017-10-02T18:19:38Z

ok thanks for reply

bchretien · 2017-10-19T19:19:05Z

@ddiakopoulos all good for me, keep me posted once your API re-work is on-par with this MR :)

svenevs

@bchretien I wanted a CMakeLists.txt as well and saw you had a PR open already. I added some comments above, take em or leave em 😉

The only other thing I would add to your CMake setup is to default to Release. Something like

# Set a default build configuration to Release
if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)
  set(CMAKE_BUILD_TYPE "Release" CACHE STRING "Choose the type of build." FORCE)
  set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS
               "Debug" "Release" "MinSizeRel" "RelWithDebInfo")
endif()

svenevs · 2017-11-12T18:32:04Z

CMakeLists.txt

@@ -0,0 +1,65 @@
+cmake_minimum_required(VERSION 3.0)


To the best of my knowledge, you need 3.1.3 for CMAKE_CXX_STANDARD. Or at least that's the first version it's actually documented in.

Good catch! 👍

svenevs · 2017-11-12T18:34:42Z

CMakeLists.txt

+include_directories("${CMAKE_SOURCE_DIR}/source")
+
+# Library
+add_library(tinyply source/tinyply.cpp)


It may be easier to, instead of include_directories, just add the header to the add_library call. It shouldn't make a huge difference either way, the primary reason for this comment is to enable shared or static libraries. It would look something like this:

# Default to shared library, allow bypass for static if desired option(TINYPLY_BUILD_SHARED "Build as a shared library?" ON) if (TINYPLY_BUILD_SHARED) set(TINYPLY_LIBRARY_TYPE "SHARED") else() set(TINYPLY_LIBRARY_TYPE "STATIC") endif() # The library consists of two files add_library( tinyply ${TINYPLY_LIBRARY_TYPE} "${CMAKE_CURRENT_SOURCE_DIR}/source/tinyply.h" "${CMAKE_CURRENT_SOURCE_DIR}/source/tinyply.cpp" )

Good point, I'll add that.

svenevs · 2017-11-12T18:39:36Z

CMakeLists.txt

+
+# Example
+add_executable(example source/example.cpp)
+target_link_libraries(example PRIVATE tinyply)


See this excellent explanation on private linkage. It doesn't surface here because example.cpp does #include "tinyply.h (as opposed to #include <tinyply.h>). In short, PRIVATE vs PUBLIC here shouldn't matter at all if you get rid of include_directories, and everything remains in the same folder (which it should). In my personal scrapped together CMake (where I don't care about installation, I build it from the parent project), I did something like

# Build the example when no parent project get_directory_property(TINYPLY_HAS_PARENT PARENT_DIRECTORY) if (NOT TINY_PLY_HAS_PARENT) add_executable(tinyply-core "${CMAKE_CURRENT_SOURCE_DIR}/source/example.cpp") target_link_libraries(tinyply-core tinyply) endif()

It may or may not be appropriate here since you aren't installing the executable (nor does it really make sense to).

👍 Indeed private linkage here is irrelevant, I don't really know why I wrote that back then...

bchretien · 2018-02-09T13:02:31Z

@svenevs thanks for the review, I completely missed the notification so sorry for the long delay! I integrated your changes, and I'll fix conflicts with master.

EDIT: apparently a lot has been going on on the 2.0 branch, I'll need to check if the bugs I fixed and the features I added are there or not, and adapt or close my MR accordingly.

ddiakopoulos · 2020-02-02T02:48:03Z

Most, if not all, of the features and bugfixes documented here have been added over the past ~2 years!

bchretien added 18 commits January 27, 2017 15:40

Add CMake support

1598dc7

- Build library and example. - Generate tinyply.pc for pkg-config support. - Generate CMake package configuration files. - Install relevant files.

Add .travis.yml

ac8f9a0

Add basic test for reading

17a1c8c

Add test for writing PLY files

5df25be

Force minimum CMake version to 3.0

d40cb28

Add missing include

a8c00af

Add include directory to generated CMake config files

c8ce1f7

Fix generated CMake config

e444752

Add support for variable-length lists

04a5fef

Add support for writing variable-length lists (ASCII)

43f4acb

Fix read_internal for variable-length lists

ba25a93

Reset offsets after reading file

2a0f217

Add some asserts

8ceccf7

Add new is_binary() method to PlyFile

837a191

Fix support of fixed-length lists

18b5b50

Remove trailing whitespaces in ASCII

5595393

Allow skipping elements entirely

4bd2d49

[ci] Fix install prefix

721ef72

Override listCount for variable-length lists

63a39e6

Fix issues detected by clang static analyzer

df5b0fc

svenevs reviewed Nov 12, 2017

View reviewed changes

caselitz mentioned this pull request Apr 26, 2018

write variable length lists (tinyply 2.0) #17

Open

ddiakopoulos closed this Feb 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various improvements #5

Various improvements #5

bchretien commented Jan 30, 2017

bchretien commented Jan 30, 2017 •

edited

Loading

bchretien commented Jan 30, 2017

ddiakopoulos commented Feb 16, 2017

maruncz commented Feb 16, 2017 •

edited

Loading

bchretien commented Feb 20, 2017 •

edited

Loading

maruncz commented Feb 20, 2017

bchretien commented Feb 20, 2017

maruncz commented Feb 20, 2017

bchretien commented Feb 20, 2017

maruncz commented Oct 2, 2017

ddiakopoulos commented Oct 2, 2017

maruncz commented Oct 2, 2017

bchretien commented Oct 19, 2017

svenevs left a comment

svenevs Nov 12, 2017

bchretien Feb 9, 2018

svenevs Nov 12, 2017

bchretien Feb 9, 2018

svenevs Nov 12, 2017

bchretien Feb 9, 2018

bchretien commented Feb 9, 2018 •

edited

Loading

ddiakopoulos commented Feb 2, 2020

Various improvements #5

Various improvements #5

Conversation

bchretien commented Jan 30, 2017

bchretien commented Jan 30, 2017 • edited Loading

bchretien commented Jan 30, 2017

ddiakopoulos commented Feb 16, 2017

maruncz commented Feb 16, 2017 • edited Loading

bchretien commented Feb 20, 2017 • edited Loading

maruncz commented Feb 20, 2017

bchretien commented Feb 20, 2017

maruncz commented Feb 20, 2017

bchretien commented Feb 20, 2017

maruncz commented Oct 2, 2017

ddiakopoulos commented Oct 2, 2017

maruncz commented Oct 2, 2017

bchretien commented Oct 19, 2017

svenevs left a comment

Choose a reason for hiding this comment

svenevs Nov 12, 2017

Choose a reason for hiding this comment

bchretien Feb 9, 2018

Choose a reason for hiding this comment

svenevs Nov 12, 2017

Choose a reason for hiding this comment

bchretien Feb 9, 2018

Choose a reason for hiding this comment

svenevs Nov 12, 2017

Choose a reason for hiding this comment

bchretien Feb 9, 2018

Choose a reason for hiding this comment

bchretien commented Feb 9, 2018 • edited Loading

ddiakopoulos commented Feb 2, 2020

bchretien commented Jan 30, 2017 •

edited

Loading

maruncz commented Feb 16, 2017 •

edited

Loading

bchretien commented Feb 20, 2017 •

edited

Loading

bchretien commented Feb 9, 2018 •

edited

Loading