Point set processing: LAS I/O by sgiraudot · Pull Request #2138 · CGAL/cgal

sgiraudot · 2017-05-24T12:23:34Z

Summary of Changes

Input/output functions for LAS format (+ updated functions for PLY).

Release Management

Affected package(s): Point_set_processing_3
Small Feature: https://cgal.geometryfactory.com/CGAL/Members/wiki/Features/Small_Features/Read_and_write_LAS_point_sets

sgiraudot · 2017-05-29T09:06:19Z

There are bugs in Travis because I forgot to update one part of the code that rely on the PLY reader. I'll fix it.

sgiraudot · 2017-05-29T13:51:28Z

Bugs should be fixed.

sgiraudot · 2017-05-29T15:01:31Z

Note (cc @lrineau): we should install LASlib on some of the platforms of the testsuite so that this new feature is tested properly (we now have a dedicated fork that should be easy to install).

lrineau · 2017-05-29T15:03:35Z

@sgiraudot @maxGimeno You should work together and propose a pull-request to some docker images at https://github.com/CGAL/cgal-testsuite-dockerfiles.

afabri · 2017-05-29T15:09:03Z

And you (Simon&Maxime) should also look with @janetournois for Windows.

lrineau · 2017-05-30T12:23:57Z

Installation/cmake/modules/UseLASLIB.cmake

 # It assumes that find_package(LASLIB) was already called.

-add_definitions(-DCGAL_LINKED_WITH_LASLIB)
+add_definitions(-DCGAL_LINKED_WITH_LASLIB -DUSE_AS_DLL)


Where does that name USE_AS_DLL come from? It should have a kind of namespacing: LASLIB_USED_AS_DLL, or
CGAL_USE_LASLIB_AS_DLL...

It's from LASlib.

We forked it, so I could change it, but I'd rather change as little things as possible in the library. Maybe I can add #define USE_AS_DLL inside the CGAL code just before including the LASlib header and #undef right after (there is exactly one inclusion of LASlib). What do you think?

If the CMake command was a target_compile_definitions, just for a specific target, that would be fine. But here the line add_definitions(-DCGAL_LINKED_WITH_LASLIB -DUSE_AS_DLL) will add -DUSE_AS_DLL to all compilation commands of the CMake project.

If that is not too complicated to hide that macro in just one header of CGAL, I would prefer.

janetournois · 2017-06-01T15:05:28Z

Sosno should be ready to test this PR on visual studio 2015.

lrineau · 2017-06-02T15:18:50Z

The Travis build failed, for a trivial reason (the license-checking macros).
More important: that PR does not compile on my machine. The error is in FindLASLIB.

lrineau · 2017-06-02T15:22:10Z

Installation/cmake/modules/FindLASLIB.cmake

+                  ENV LASLIB_LIB_DIR
+            )
+
+if(LASLIB_LIBRARIES)


If LASLIB_LIBRARIES evaluates to TRUE, but not LASZIP_INCLUDE_DIR, there is a CMake error.

On my machine, there is a file /usr/lib64/liblas.so, from the package liblas-devel-1.8.0-13.fc25.x86_64, that corresponds to http://www.liblas.org, and LASLIB_LIBRARIES is automatically set to /usr/lib64/liblas.so, but LASZIP_INCLUDE_DIR remains NOTFOUND.

The main problem here is that libLAS != LASlib, although both libraries use the same filename… so anyway, there's no way the code will compile, even if we add LASzip. I'm not sure at all how we can handle that.

I think LASLIB_FOUND should not be set to true if LASLIB_INCLUDE_DIR is not-found. That would probably solve the issue I have had.

sgiraudot · 2017-06-07T09:13:40Z

I added the test for LASLIB_INCLUDE_DIR.

lrineau · 2017-06-07T09:18:16Z

That works correctly now on my machine: instead of a CMake critical error, I just have the message

the LAS IO plugin needs LAS libraries and will not be compiled.

as expected.

lrineau · 2017-06-07T09:25:00Z

It seems that PR change the API that was introduced in CGAL-4.8 by Small_Features/Point_set_processing_ply_io and modified in CGAL-4.9 by Small_Features/Extend_PLY_point_set_reader. What is the upgrade path for users of that documented API? If that is a simple breakable, then the changes.html should mention the breakage, with a bold Breaking change: in the text.

sgiraudot · 2017-06-07T09:51:43Z

Good point. I've updated changes.html.

lrineau · 2017-06-07T10:01:09Z

Installation/changes.html

+    points with properties is modified for unification with LAS
+    (see <code>CGAL::read_ply_points_with_properties()</code>). A new
+    function to write PLY with properties is provided
+    (<code>CGAL::write_las_points_with_properties()</code>).</li>


I think you have swapped the two names of functions CGAL::read_ply_points_with_properties()/CGAL::write_las_points_with_properties() in the description.

lrineau · 2017-06-07T10:03:12Z

The Travis error will remain:

The following files do not have an include directive of the package license header:
Point_set_processing_3/include/CGAL/IO/write_las_points.h
Point_set_processing_3/include/CGAL/IO/read_las_points.h

https://travis-ci.org/CGAL/cgal/jobs/238376800#L4070

The two files should include:

#include <CGAL/license/Point_set_processing_3.h>

sgiraudot · 2017-06-07T10:05:57Z

Alright, I fixed both problems (error in changes.html + license check).

lrineau · 2017-06-07T19:49:07Z

Doxygen error:

.../doc/Documentation/Installation.txt:576: Warning: Unexpected html tag <span> found within <a href=...> context
.../doc/Documentation/Installation.txt:576: Warning: Unexpected html tag </span> found within <a href=...> context

https://cgal.geometryfactory.com/CGAL/Manual_doxygen_test/CGAL-4.11-Ic-35/Documentation.log

… ICC" We have to check `__GXX_EXPERIMENTAL_CXX0X__`. The issue in Boost-1.53 is that the other macro `__GXX_EXPERIMENTAL_CPP0X__` is checked instead.

…r messages

…tures with ICC

sgiraudot · 2017-07-11T10:00:59Z

The static assertion should be fixed and the branch is rebased.

sgiraudot · 2017-07-12T06:15:41Z

Everything looks good except for the remaining warning on Windows DLL. I deactivated it in the code: it comes from LASlib and I'd rather change our fork of LASlib as little as possible (and this warning seems to require many additions of the export DLL mechanism). Anyway, we already deactivated a warning from GCC for the inclusion of LASlib…

sgiraudot · 2017-07-13T08:38:39Z

The remaining error on Windows should be fixed.

Point set processing: LAS I/O

sgiraudot added Pkg::Point_set_processing_3 pre-approved For pre-approved small features. After 15 days the feature will be accepted. Small feature labels May 24, 2017

sgiraudot mentioned this pull request May 24, 2017

LAS reader sgiraudot/cgal#9

Closed

lrineau reviewed May 30, 2017

View reviewed changes

lrineau added Under Testing Tests failing and removed Under Testing labels Jun 1, 2017

lrineau requested changes Jun 2, 2017

View reviewed changes

lrineau approved these changes Jun 7, 2017

View reviewed changes

lrineau reviewed Jun 7, 2017

View reviewed changes

lrineau added Ready to be tested Under Testing and removed Tests failing labels Jun 7, 2017

lrineau self-assigned this Jun 7, 2017

lrineau added the Tests failing label Jun 7, 2017

sgiraudot and others added 14 commits July 11, 2017 11:58

Use std:: everywhere for tuples (make_tuple, get, etc.)

fbe9e78

Fix: use cpp11::tuple in examples

238f3af

add missing includes

dce27d7

use the import instruction of LasReader_LAS

b4bf70c

Fix binary write bug

eed458b

Fix CMakeLists for Point_set_3 examples

a576d01

Add second missing ::

09c11ca

Add hack in config.h for boost detection of CXX11 features with ICC

6bc9fb5

fix: "Add hack in config.h for boost detection of CXX11 features with…

33871ea

… ICC" We have to check `__GXX_EXPERIMENTAL_CXX0X__`. The issue in Boost-1.53 is that the other macro `__GXX_EXPERIMENTAL_CPP0X__` is checked instead.

Add non-CXX11 overloads of PLY/LAS IO for Point_set_3 to display erro…

914a6fe

…r messages

Remove useless include in example

99bd05e

Warning fix: missing return

66cd564

second fix for: Add hack in config.h for boost detection of CXX11 fea…

931a0a2

…tures with ICC

Fix static assertion by using a templated tag false

56919f9

sgiraudot force-pushed the Point_set_processing-Las_IO-GF branch from 1807376 to 56919f9 Compare July 11, 2017 10:00

sgiraudot mentioned this pull request Jul 11, 2017

Read PLY polygon soup #2249

Merged

Deactivate visual studio warning on LASlib DLL

41edf00

Fix wrong inclusions/warning protections

c59454e

lrineau added the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Jul 17, 2017

lrineau merged commit c59454e into CGAL:master Jul 17, 2017

lrineau added a commit that referenced this pull request Jul 17, 2017

Merge pull request #2138 from sgiraudot/Point_set_processing-Las_IO-GF

4594430

Point set processing: LAS I/O

lrineau added Tested and removed rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' Ready to be tested Under Testing labels Jul 17, 2017

lrineau deleted the Point_set_processing-Las_IO-GF branch July 17, 2017 12:04

sgiraudot mentioned this pull request Aug 3, 2017

Create CMake list support for LAStools #1846

Closed

Conversation

sgiraudot commented May 24, 2017

Summary of Changes

Release Management

Uh oh!

sgiraudot commented May 29, 2017

Uh oh!

sgiraudot commented May 29, 2017

Uh oh!

sgiraudot commented May 29, 2017

Uh oh!

lrineau commented May 29, 2017

Uh oh!

afabri commented May 29, 2017

Uh oh!

lrineau May 30, 2017

Choose a reason for hiding this comment

Uh oh!

sgiraudot May 30, 2017

Choose a reason for hiding this comment

Uh oh!

lrineau May 30, 2017

Choose a reason for hiding this comment

Uh oh!

janetournois commented Jun 1, 2017

Uh oh!

lrineau commented Jun 2, 2017

Uh oh!

lrineau Jun 2, 2017

Choose a reason for hiding this comment

Uh oh!

sgiraudot Jun 6, 2017

Choose a reason for hiding this comment

Uh oh!

lrineau Jun 6, 2017

Choose a reason for hiding this comment

Uh oh!

sgiraudot commented Jun 7, 2017

Uh oh!

lrineau commented Jun 7, 2017

Uh oh!

lrineau commented Jun 7, 2017

Uh oh!

sgiraudot commented Jun 7, 2017

Uh oh!

lrineau Jun 7, 2017

Choose a reason for hiding this comment

Uh oh!

lrineau commented Jun 7, 2017

Uh oh!

sgiraudot commented Jun 7, 2017

Uh oh!

lrineau commented Jun 7, 2017

Uh oh!

sgiraudot commented Jul 11, 2017

Uh oh!

sgiraudot commented Jul 12, 2017

Uh oh!

sgiraudot commented Jul 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants