LasIO.jl icon indicating copy to clipboard operation
LasIO.jl copied to clipboard

Las1.4 support with all point formats

Open msbahal opened this issue 5 years ago • 4 comments

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

Still TODOs:

  • This PR doesn't allow use of the waveform data (future work).
  • In this version, I have added a Laszip executable in the repo. I would really like laszip to be built locally in the future.

Thank you for your time and feedback in advance!

msbahal avatar Nov 23 '20 01:11 msbahal

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)

c42f avatar Nov 23 '20 02:11 c42f

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)?

msbahal avatar Nov 23 '20 06:11 msbahal

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.

visr avatar Nov 23 '20 12:11 visr

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.

evetion avatar Nov 23 '20 14:11 evetion