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.

Sign up for GitHub

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

Jump to bottom

r.in.pdal – a PDAL based replacement of r.in.lidar #1200

Merged
merged 44 commits into from
Jul 15, 2021
Merged

r.in.pdal – a PDAL based replacement of r.in.lidar #1200

merged 44 commits into from
Jul 15, 2021

Conversation

marisn
Copy link
Contributor

@marisn marisn commented Dec 21, 2020 •
edited
Loading

This is an enhanced version of r.in.pdal based on https://github.com/wenzeslaus/grass/tree/pdal-binning (PR #61). This PR makes PR #61 obsolete. This PR depends on #1148 Code has been rebased on top of current master for easier testing.

TODO:

  • Print file info (-p flag)
  • More tests for filters, selection, base_raster, ...
  • Review for missing functionality in comparison to r.in.lidar

Features to discuss:

  • Determine extent in scan mode (with all filters applied) (-s flag) Postponed after 8.0
  • Output convex hull of imported data as a vector map (@neteler wish) Postponed after 8.0
  • Add PDAL pipeline support (on-fly preprocessing) Postponed after 8.0

@marisn
Copy link
Contributor Author

marisn commented Dec 25, 2020

@wenzeslaus could you, please, take a look into -p flag? My C++-fu is rather weak. With -p in place, I would call it a day.

@wenzeslaus wenzeslaus self-requested a review March 31, 2021 01:07
@neteler
Copy link
Member

neteler commented May 15, 2021

We may drop my wish "Output convex hull of imported data as a vector map".

Almost there, it would be great to see this PR be merged.

@marisn marisn requested review from neteler and nilason June 10, 2021 16:07
@marisn marisn marked this pull request as ready for review June 10, 2021 16:08
@marisn
Copy link
Contributor Author

marisn commented Jun 10, 2021

Not perfect, but releasing 8.0 without LAS support is a no-no. There always will be 8.2 for improvements.

wenzeslaus
wenzeslaus reviewed Jun 11, 2021
View reviewed changes
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, I can't dedicate to this review the time it deserves. Given the v8 timing and the broad testing this code will eventually get by many including myself, I suggest to merge it. I'm the author of the original code, so I think that code is at least okayish and I only gave the new code a couple of causal looks at different times.

Some tests rely on v.out.lidar, so they are failing. They need to be excluded in .gunittest.cfg.

@marisn marisn mentioned this pull request Jun 11, 2021
Closed
@neteler
Copy link
Member

neteler commented Jun 11, 2021

Some tests rely on v.out.lidar, so they are failing. They need to be excluded in .gunittest.cfg.

I'd suggest to simply add rinpdal_basic_points.las/laz to data/ and keep the test active (locally tested ok) by removing the v.random and v.out.lidar lines in test_r_in_pdal_basic.py.

File: rinpdal_basic_points_las_laz.zip

@marisn
Copy link
Contributor Author

marisn commented Jun 11, 2021

I'd suggest to simply add rinpdal_basic_points.las/laz to data/ and keep the test active (locally tested ok) by removing the v.random and v.out.lidar lines in test_r_in_pdal_basic.py.

No need as the functionality of binning is already tested in the test_r_in_pdal_binning.py file.

@neteler neteler mentioned this pull request Jun 12, 2021
Merged
neteler
neteler reviewed Jun 13, 2021
View reviewed changes
raster/r.in.pdal/main.cpp Outdated Show resolved Hide resolved
neteler
neteler reviewed Jun 13, 2021
View reviewed changes
raster/r.in.pdal/lidar.h Outdated Show resolved Hide resolved
@marisn
Copy link
Contributor Author

marisn commented Jun 18, 2021

It turns out r.in.pdal needs a specific minimum version of pdal library. I changed the autoconf part (3d12173) but now there is no pdal support for older versions at all (Ubuntu 18.04). How should we proceed? Keep it as-is or introduce a separate check for r.in.pdal needs? If there is a separate check for r.in.pdal compatible pdal version, should it be listed in the summary of configure run?

@marisn marisn changed the title WIP: r.in.pdal – a PDAL based replacement of r.in.lidar r.in.pdal – a PDAL based replacement of r.in.lidar Jun 19, 2021
@wenzeslaus @marisn
r.in.pdal: point cloud binning with PDAL
b468012 
Reads points using PDAL, uses several PDAL filters including reprojection,
filters using custom filter, writes into memory structures.

Reuses r.in.lidar functionality with moving code into PDAL Filter
and Writer classes. Preserves most of r.in.lidar functionality,
but misses several features most notably scanning and auto extent.
Furthermore, it now assumes that the whole output raster fits
into memory. The multi-pass code was removed and it should be
replaced by use of the improved Segment with all-in-memory mode.
@wenzeslaus @marisn
Basic test of r.in.pdal
921c296
@wenzeslaus @marisn
Pass PDAL C++ options only to C++ files
51be759
@wenzeslaus @marisn
Fix unused parameter of update_bin_index fun
6831c91
@wenzeslaus @marisn
Use a macro to create format strings for a typedef
c909f0c
@wenzeslaus @marisn
ISO C++ forbids converting a string constant to char*
e7adf97
marisn added 13 commits July 13, 2021 18:42
@wenzeslaus wenzeslaus mentioned this pull request Jul 14, 2021
Closed
1 task
@marisn marisn merged commit f752840 into OSGeo:master Jul 15, 2021
wenzeslaus added a commit to wenzeslaus/grass that referenced this pull request Jul 20, 2021
@wenzeslaus
ci: Remove duplication introduced with PDAL
862126d 
matrix.os has the name of the the system which is used in the YAML.
This basically returns the CI YAML files state to what it was before OSGeo#1200
keeping the addition of a new build script and version-specific naming for the scripts.
@wenzeslaus wenzeslaus mentioned this pull request Jul 20, 2021
Merged
@marisn marisn deleted the wenzeslaus-pdal-binning branch July 30, 2021 16:38
@neteler neteler added this to the 8.0.0 milestone Dec 9, 2021
@neteler neteler mentioned this pull request May 27, 2022
Open
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
@marisn @wenzeslaus @ninsbl
r.in.pdal – a PDAL based replacement of r.in.lidar (OSGeo#1200)
374b468 
* r.in.pdal: point cloud binning with PDAL

Reads points using PDAL, uses several PDAL filters including reprojection,
filters using custom filter, writes into memory structures.

Reuses r.in.lidar functionality with moving code into PDAL Filter
and Writer classes. Preserves most of r.in.lidar functionality,
but misses several features most notably scanning and auto extent.
Furthermore, it now assumes that the whole output raster fits
into memory. The multi-pass code was removed and it should be
replaced by use of the improved Segment with all-in-memory mode.

In comparison to r.in.lidar, here are more binning options, including eigenvalue statistic option according to suggestion of "Mallet et al. 2011. Relevance assessment of full-waveform lidar data for urban area classification"; Summation is performed with Neumaiers improved Kahan–Babuska algorithm; Variance, stddev and coeff_var with Welfords algorithm.

Testing infrastructure is also updated to better handle minimal version required of PDAL library.

Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com>
Co-authored-by: Māris Nartišs <maris.nartiss@lu.lv>
@neteler neteler mentioned this pull request Jan 18, 2023
Merged
neteler pushed a commit that referenced this pull request Jan 18, 2023
@anikaweinmann
Add r.in.pdal test for alpine image (#2758)
cf284fa 
* add r.in.pdal test for alpine image
* fix r.in.pdal tests
* removement of r.in.pdal addon installation in Ubuntu and Debian docker images (as being replaced in C++ version in #1200)

Co-authored-by: anikaweinmann <aweinmann@mundialis.de>
neteler pushed a commit that referenced this pull request Jan 18, 2023
@anikaweinmann @neteler
Add r.in.pdal test for alpine image (#2758)
13b3420 
* add r.in.pdal test for alpine image
* fix r.in.pdal tests
* removement of r.in.pdal addon installation in Ubuntu and Debian docker images (as being replaced in C++ version in #1200)

Co-authored-by: anikaweinmann <aweinmann@mundialis.de>
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
@marisn @wenzeslaus @ninsbl
r.in.pdal – a PDAL based replacement of r.in.lidar (OSGeo#1200)
e1d6f32 
* r.in.pdal: point cloud binning with PDAL

Reads points using PDAL, uses several PDAL filters including reprojection,
filters using custom filter, writes into memory structures.

Reuses r.in.lidar functionality with moving code into PDAL Filter
and Writer classes. Preserves most of r.in.lidar functionality,
but misses several features most notably scanning and auto extent.
Furthermore, it now assumes that the whole output raster fits
into memory. The multi-pass code was removed and it should be
replaced by use of the improved Segment with all-in-memory mode.

In comparison to r.in.lidar, here are more binning options, including eigenvalue statistic option according to suggestion of "Mallet et al. 2011. Relevance assessment of full-waveform lidar data for urban area classification"; Summation is performed with Neumaiers improved Kahan–Babuska algorithm; Variance, stddev and coeff_var with Welfords algorithm.

Testing infrastructure is also updated to better handle minimal version required of PDAL library.

Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com>
Co-authored-by: Māris Nartišs <maris.nartiss@lu.lv>
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
@anikaweinmann @ninsbl
Add r.in.pdal test for alpine image (OSGeo#2758)
ae1cef4 
* add r.in.pdal test for alpine image
* fix r.in.pdal tests
* removement of r.in.pdal addon installation in Ubuntu and Debian docker images (as being replaced in C++ version in OSGeo#1200)

Co-authored-by: anikaweinmann <aweinmann@mundialis.de>
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
@anikaweinmann @neteler
Add r.in.pdal test for alpine image (OSGeo#2758)
cd3f7d2 
* add r.in.pdal test for alpine image
* fix r.in.pdal tests
* removement of r.in.pdal addon installation in Ubuntu and Debian docker images (as being replaced in C++ version in OSGeo#1200)

Co-authored-by: anikaweinmann <aweinmann@mundialis.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neteler neteler neteler approved these changes

@wenzeslaus wenzeslaus wenzeslaus left review comments

@nilason nilason Awaiting requested review from nilason

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marisn @neteler @wenzeslaus