search

visr / LasIO.jl

Julia package for reading and writing the LAS lidar format.
Other
22 stars 13 forks source link

Las1.4 support with all point formats #37

Open msbahal opened 3 years ago

msbahal commented 3 years ago

This PR enables reading and writing of Las files version 1.4 and all PointFormats 0 - 10.

Still TODOs:

Thank you for your time and feedback in advance!

c42f commented 3 years ago

Just a quick note:

In this version, I have added a Laszip executable in the repo

It seems @evetion has made https://github.com/evetion/LazIO.jl for laz support, so presumably you should use that for laz integration.

(For laszip libraries see the build recipe at https://github.com/JuliaPackaging/Yggdrasil/pull/1079)

msbahal commented 3 years ago

Just a quick note:

In this version, I have added a Laszip executable in the repo

It seems @evetion has made https://github.com/evetion/LazIO.jl for laz support, so presumably you should use that for laz integration.

(For laszip libraries see the build recipe at JuliaPackaging/Yggdrasil#1079)

I initially did attemp to integrate all this with LazIO. However, very quickly, it got a little too complicated for me. Also, when I read the raw LasPoint6 data using LazIO, I was getting weird data errors (return number was > number of return). Obviously I was doing something wrong there. I found working with executable extremely simple so I complemented the, already existing, LAZ functionality of LasIO instead. Gave the cleanest looking code.

I did want to ask, why don’t we add a laszip executable as part of Laszip_jll package? Also can we bump this package to be compatible with julia > 1.3 so we can use jll packages (like LazIO)?

visr commented 3 years ago

I did want to ask, why don’t we add a laszip executable as part of Laszip_jll package?

I think it would be good to add this. It may be as simple as adding an ExecutableProduct to https://github.com/JuliaPackaging/Yggdrasil/tree/master/L/LASzip, similar to how it is done in https://github.com/JuliaPackaging/Yggdrasil/blob/master/G/GDAL/build_tarballs.jl. Feel free to try it out. I would prefer that over a build script. Bumping compat to Julia 1.3 for this package is no problem.

evetion commented 3 years ago

I initially did attempt to integrate all this with LazIO. However, very quickly, it got a little too complicated for me. Also, when I read the raw LasPoint6 data using LazIO, I was getting weird data errors (return number was > number of return). Obviously I was doing something wrong there.

Feel free to make an issue over at LazIO for such things. Could be bugs in laszip, but the changes in the spec can muddle things as well, especially if you have non-compliant .las/z files.

I found working with executable extremely simple so I complemented the, already existing, LAZ functionality of LasIO instead. Gave the cleanest looking code. I did want to ask, why don’t we add a laszip executable as part of Laszip_jll package?

I agree with adding laszip to the .jll. Note however that this method of conversion doesn't support indexing or other fancy streaming methods, which is the reason we've made LazIO.

Overall, we're very happy with PRs, but I would advise to coordinate a bit more in the future, so we can better help you. For example, there's an defunct PR over at #16 (itself based on work by @c42f), that could provide some inspiration, as it also includes some waveform parts and extended vlrs.