From eab583b23d79a7c39742bc52d9bbf74f68c9ba02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C4=81ris=20Narti=C5=A1s?= Date: Sun, 24 Nov 2024 14:41:23 +0200 Subject: [PATCH 1/2] r.in.pdal: Improve PDAL error handling Some non-conformant LAS files trigger an error in PDAL library that results in an unhandled exit from r.in.pdal module. This code catches all PDAL errors and reports them in a GRASS way. One of error sources is lack of SRS information in the imported LAS file. This code propogates SRS check override from GRASS to PDAL library thus eliminating an error observed by non-conformant 1.4 LAS files. Fixes: https://github.com/OSGeo/grass/issues/4157 --- raster/r.in.pdal/info.cpp | 37 ++++-- raster/r.in.pdal/info.h | 6 +- raster/r.in.pdal/main.cpp | 26 ++-- .../testsuite/test_r_in_pdal_print.py | 113 ++++++++++++++++++ 4 files changed, 164 insertions(+), 18 deletions(-) create mode 100644 raster/r.in.pdal/testsuite/test_r_in_pdal_print.py diff --git a/raster/r.in.pdal/info.cpp b/raster/r.in.pdal/info.cpp index 47ebd01fe28..7fb638d8fc2 100644 --- a/raster/r.in.pdal/info.cpp +++ b/raster/r.in.pdal/info.cpp @@ -13,7 +13,8 @@ #include void get_extent(struct StringList *infiles, double *min_x, double *max_x, - double *min_y, double *max_y, double *min_z, double *max_z) + double *min_y, double *max_y, double *min_z, double *max_z, + bool nosrs) { pdal::StageFactory factory; bool first = 1; @@ -25,15 +26,25 @@ void get_extent(struct StringList *infiles, double *min_x, double *max_x, std::string pdal_read_driver = factory.inferReaderDriver(infile); if (pdal_read_driver.empty()) - G_fatal_error("Cannot determine input file type of <%s>", infile); + G_fatal_error(_("Cannot determine input file type of <%s>"), + infile); pdal::PointTable table; pdal::Options las_opts; pdal::Option las_opt("filename", infile); las_opts.add(las_opt); + if (nosrs) { + pdal::Option nosrs_opt("nosrs", true); + las_opts.add(nosrs_opt); + } pdal::LasReader las_reader; las_reader.setOptions(las_opts); - las_reader.prepare(table); + try { + las_reader.prepare(table); + } + catch (const std::exception &err) { + G_fatal_error(_("PDAL error: %s"), err.what()); + } const pdal::LasHeader &las_header = las_reader.header(); if (first) { *min_x = las_header.minX(); @@ -62,16 +73,16 @@ void get_extent(struct StringList *infiles, double *min_x, double *max_x, } } -void print_extent(struct StringList *infiles) +void print_extent(struct StringList *infiles, bool nosrs) { double min_x, max_x, min_y, max_y, min_z, max_z; - get_extent(infiles, &min_x, &max_x, &min_y, &max_y, &min_z, &max_z); + get_extent(infiles, &min_x, &max_x, &min_y, &max_y, &min_z, &max_z, nosrs); fprintf(stdout, "n=%f s=%f e=%f w=%f b=%f t=%f\n", max_y, min_y, max_x, min_x, min_z, max_z); } -void print_lasinfo(struct StringList *infiles) +void print_lasinfo(struct StringList *infiles, bool nosrs) { pdal::StageFactory factory; pdal::MetadataNode meta_node; @@ -86,15 +97,25 @@ void print_lasinfo(struct StringList *infiles) std::string pdal_read_driver = factory.inferReaderDriver(infile); if (pdal_read_driver.empty()) - G_fatal_error("Cannot determine input file type of <%s>", infile); + G_fatal_error(_("Cannot determine input file type of <%s>"), + infile); pdal::PointTable table; pdal::Options las_opts; pdal::Option las_opt("filename", infile); las_opts.add(las_opt); + if (nosrs) { + pdal::Option nosrs_opt("nosrs", true); + las_opts.add(nosrs_opt); + } pdal::LasReader las_reader; las_reader.setOptions(las_opts); - las_reader.prepare(table); + try { + las_reader.prepare(table); + } + catch (const std::exception &err) { + G_fatal_error(_("PDAL error: %s"), err.what()); + } const pdal::LasHeader &h = las_reader.header(); pdal::PointLayoutPtr point_layout = table.layout(); const pdal::Dimension::IdList &dims = point_layout->dims(); diff --git a/raster/r.in.pdal/info.h b/raster/r.in.pdal/info.h index 7f8d0138ab1..546d25e3c3c 100644 --- a/raster/r.in.pdal/info.h +++ b/raster/r.in.pdal/info.h @@ -34,8 +34,8 @@ extern "C" { } void get_extent(struct StringList *, double *, double *, double *, double *, - double *, double *); -void print_extent(struct StringList *); -void print_lasinfo(struct StringList *); + double *, double *, bool); +void print_extent(struct StringList *, bool); +void print_lasinfo(struct StringList *, bool); #endif // INFO_H diff --git a/raster/r.in.pdal/main.cpp b/raster/r.in.pdal/main.cpp index 36d09558eea..15e6f260996 100644 --- a/raster/r.in.pdal/main.cpp +++ b/raster/r.in.pdal/main.cpp @@ -446,12 +446,12 @@ int main(int argc, char *argv[]) /* If we print extent, there is no need to validate rest of the input */ if (print_extent_flag->answer) { - print_extent(&infiles); + print_extent(&infiles, over_flag->answer); exit(EXIT_SUCCESS); } if (print_info_flag->answer) { - print_lasinfo(&infiles); + print_lasinfo(&infiles, over_flag->answer); exit(EXIT_SUCCESS); } @@ -507,7 +507,8 @@ int main(int argc, char *argv[]) if (extents_flag->answer) { double min_x, max_x, min_y, max_y, min_z, max_z; - get_extent(&infiles, &min_x, &max_x, &min_y, &max_y, &min_z, &max_z); + get_extent(&infiles, &min_x, &max_x, &min_y, &max_y, &min_z, &max_z, + over_flag->answer); region.east = xmax = max_x; region.west = xmin = min_x; @@ -711,16 +712,22 @@ int main(int argc, char *argv[]) std::string pdal_read_driver = factory.inferReaderDriver(infile); if (pdal_read_driver.empty()) - G_fatal_error("Cannot determine input file type of <%s>", infile); + G_fatal_error(_("Cannot determine input file type of <%s>"), + infile); pdal::Options las_opts; pdal::Option las_opt("filename", infile); las_opts.add(las_opt); + if (over_flag->answer) { + pdal::Option nosrs_opt("nosrs", true); + las_opts.add(nosrs_opt); + } // stages created by factory are destroyed with the factory pdal::Stage *reader = factory.createStage(pdal_read_driver); if (!reader) - G_fatal_error("PDAL reader creation failed, a wrong format of <%s>", - infile); + G_fatal_error( + _("PDAL reader creation failed, a wrong format of <%s>"), + infile); reader->setOptions(las_opts); readers.push_back(reader); merge_filter.setInput(*reader); @@ -779,7 +786,12 @@ int main(int argc, char *argv[]) // consumption, so using 10k in case it is faster for some cases pdal::point_count_t point_table_capacity = 10000; pdal::FixedPointTable point_table(point_table_capacity); - binning_writer.prepare(point_table); + try { + binning_writer.prepare(point_table); + } + catch (const std::exception &err) { + G_fatal_error(_("PDAL error: %s"), err.what()); + } // getting projection is possible only after prepare if (over_flag->answer) { diff --git a/raster/r.in.pdal/testsuite/test_r_in_pdal_print.py b/raster/r.in.pdal/testsuite/test_r_in_pdal_print.py new file mode 100644 index 00000000000..2c613ab88e2 --- /dev/null +++ b/raster/r.in.pdal/testsuite/test_r_in_pdal_print.py @@ -0,0 +1,113 @@ +""" +Name: r.in.pdal info printing and error handling tests +Purpose: Validates output of LAS file property printing and handling + of broken LAS files + +Author: Maris Nartiss +Copyright: (C) 2024 by Maris Nartiss and the GRASS Development Team +Licence: This program is free software under the GNU General Public + License (>=v2). Read the file COPYING that comes with GRASS + for details. +""" + +import os +import pathlib +import shutil +import unittest +from tempfile import TemporaryDirectory + +from grass.script import core as grass +from grass.script import read_command +from grass.gunittest.case import TestCase +from grass.gunittest.main import test + + +class InfoTest(TestCase): + """ + Test printing of extent and metadata + + This test requires pdal CLI util to be available. + """ + + @classmethod + @unittest.skipIf(shutil.which("pdal") is None, "Cannot find pdal utility") + def setUpClass(cls): + """Ensures expected computational region and generated data""" + cls.use_temp_region() + cls.runModule("g.region", n=18, s=0, e=18, w=0, res=6) + + cls.data_dir = os.path.join(pathlib.Path(__file__).parent.absolute(), "data") + cls.point_file = os.path.join(cls.data_dir, "points.csv") + cls.tmp_dir = TemporaryDirectory() + cls.las_file = os.path.join(cls.tmp_dir.name, "points.las") + grass.call( + [ + "pdal", + "translate", + "-i", + cls.point_file, + "-o", + cls.las_file, + "-r", + "text", + "-w", + "las", + "--writers.las.format=0", + "--writers.las.extra_dims=all", + "--writers.las.minor_version=4", + ] + ) + cls.broken_las = os.path.join(cls.tmp_dir.name, "broken.las") + pathlib.Path(cls.broken_las).write_bytes(b"LASF") + + @classmethod + def tearDownClass(cls): + """Remove the temporary region and generated data""" + cls.tmp_dir.cleanup() + cls.del_temp_region() + + @unittest.skipIf(shutil.which("r.in.pdal") is None, "Cannot find r.in.pdal") + def test_extent_bad(self): + """A broken LAS file should result in an error""" + self.assertModuleFail("r.in.pdal", input=self.broken_las, flags="g", quiet=True) + + @unittest.skipIf(shutil.which("r.in.pdal") is None, "Cannot find r.in.pdal") + def test_info_bad(self): + """A broken LAS file should result in an error""" + self.assertModuleFail("r.in.pdal", input=self.broken_las, flags="p", quiet=True) + + @unittest.skipIf(shutil.which("r.in.pdal") is None, "Cannot find r.in.pdal") + def test_extent_good(self): + """Reported extent should match provided data""" + out = read_command("r.in.pdal", input=self.las_file, flags="g", quiet=True) + for kvp in out.strip().split(" "): + key, value = kvp.split("=") + if key == "n": + self.assertAlmostEqual(float(value), 17, places=6) + continue + if key == "s": + self.assertAlmostEqual(float(value), 1, places=6) + continue + if key == "e": + self.assertAlmostEqual(float(value), 17, places=6) + continue + if key == "w": + self.assertAlmostEqual(float(value), 1, places=6) + continue + if key == "t": + self.assertAlmostEqual(float(value), 28, places=6) + continue + if key == "b": + self.assertAlmostEqual(float(value), 1, places=6) + + @unittest.skipIf(shutil.which("r.in.pdal") is None, "Cannot find r.in.pdal") + def test_info_good(self): + """Validate successful file info printing""" + out = read_command("r.in.pdal", input=self.las_file, flags="p", quiet=True) + self.assertIn("File version = 1.4", out) + self.assertIn("File signature: LASF", out) + self.assertIn("Point count: 53", out) + + +if __name__ == "__main__": + test() From b4752e4a8ef3b3ce36951b0b8ea38dd62d6eebef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C4=81ris=20Narti=C5=A1s?= Date: Sun, 24 Nov 2024 14:49:23 +0200 Subject: [PATCH 2/2] r.in.pdal: update copyright notice --- raster/r.in.pdal/info.cpp | 2 +- raster/r.in.pdal/main.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/raster/r.in.pdal/info.cpp b/raster/r.in.pdal/info.cpp index 7fb638d8fc2..b986c6fba47 100644 --- a/raster/r.in.pdal/info.cpp +++ b/raster/r.in.pdal/info.cpp @@ -1,7 +1,7 @@ /* * r.in.pdal Functions printing out various information on input LAS files * - * Copyright 2021 by Maris Nartiss, and The GRASS Development Team + * Copyright 2021-2024 by Maris Nartiss, and The GRASS Development Team * Author: Maris Nartiss * * This program is free software licensed under the GPL (>=v2). diff --git a/raster/r.in.pdal/main.cpp b/raster/r.in.pdal/main.cpp index 15e6f260996..27d5cfcd574 100644 --- a/raster/r.in.pdal/main.cpp +++ b/raster/r.in.pdal/main.cpp @@ -4,12 +4,12 @@ * * AUTHOR(S): Vaclav Petras * Based on r.in.xyz and r.in.lidar by Markus Metz, - * Hamish Bowman, Volker Wichmann + * Hamish Bowman, Volker Wichmann, Maris Nartiss * * PURPOSE: Imports LAS LiDAR point clouds to a raster map using * aggregate statistics. * - * COPYRIGHT: (C) 2019-2021 by Vaclav Petras and the GRASS Development Team + * COPYRIGHT: (C) 2019-2024 by Vaclav Petras and the GRASS Development Team * * This program is free software under the GNU General Public * License (>=v2). Read the file COPYING that comes with