Option to crop interest zone and filter min dbh trees #22
Conversation
|
@thomas-lowe could I get a review? |
| @@ -23,16 +24,24 @@ bool readLas(const std::string &file_name, | |||
| { | |||
| #if RAYLIB_WITH_LAS | |||
| std::cout << "readLas: filename: " << file_name << std::endl; | |||
| std::ifstream file(file_name); | |||
|
|
|||
| if (file) | |||
There was a problem hiding this comment.
This seems unnecessary, there is already a check and message below to cerr that says "failed to open stream".
There was a problem hiding this comment.
fail() method call below does not provide information if the file does not exists, I think that the error is quite generic and is not very helpful while working with docker. One has to mount the volumes correctly and it took me time to understand and realise that file did not exist in the image as the volume was not mounted correctly. I am happy to refactor the code and add proper error handling?
| @@ -46,11 +46,16 @@ int rayImport(int argc, char *argv[]) | |||
| bool ray_format = | |||
| ray::parseCommandLine(argc, argv, { &cloud_file, &ray_text, &ray_vec }, { &max_intensity_option, &remove }); | |||
| if (!standard_format && !position_format && !ray_format) | |||
| { | |||
There was a problem hiding this comment.
Not sure why rayimport.cpp has clang changes, as I don't see any other change to this file.
I assume clang changes are only to files you have changed in some other functional way.
I'd prefer these files weren't reformatted like this.
There was a problem hiding this comment.
I have restored the previous style. I believe that either .clang-format file needs to be removed or the whole repo formatted 🤔
| @@ -55,12 +57,14 @@ void usage(int exit_code = 1) | |||
| std::cout << " --crop_length 1.0 - (-p) crops small branches to this distance from end" << std::endl; | |||
| std::cout << " --distance_limit 1 - (-d) maximum distance between neighbour points in a tree" << std::endl; | |||
| std::cout << " --height_min 2 - (-h) minimum height counted as a tree" << std::endl; | |||
| std::cout << " --min_radius 0.01 - (-r) minimum tree radius to consider" << std::endl; | |||
There was a problem hiding this comment.
This serves a very similar purpose to height_min, do you really need to crop specifically on radius rather than height?
One reason we have treetools (https://github.com/csiro-robotics/treetools) is to keep rayextract commandline params to a minimum. Could you achieve the same effect by running treesplit output_trees.txt radius 0.01 ?
If so then I would prefer we didn't add the extra parameter here.
There was a problem hiding this comment.
I think that it is quite handy functionality allowing easy trees filtering. Normally, especially when estimating the biomass, trees with dbh<10cm are ignored and this flag gives direct, non convoluted way of filtering such trees.
GabijaBern
force-pushed
the
new_command_line_params
branch
20 times, most recently
from
86779b2 to
5817ce5
Compare
|
Just those two changes above |
|
Could you point me to what exactly you are referring to? |
|
Hi Gabi, Let me know if you have any issues with this adjusted implementation. I have to be very careful to get the interface right as there are a lot of users. I have tested it on some basic examples using the overlap grid and the grid_width/grid_origin options. |
GabijaBern
force-pushed
the
new_command_line_params
branch
3 times, most recently
from
88ad782 to
aa001fb
Compare
GabijaBern
force-pushed
the
new_command_line_params
branch
19 times, most recently
from
5b42960 to
1500719
Compare
GabijaBern
force-pushed
the
new_command_line_params
branch
from
1500719 to
10fcc4a
Compare
A couple of small changes: