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.

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

Las1.4 support with all point formats #37

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

msbahal
Copy link

@msbahal msbahal commented Nov 23, 2020

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!

Mandeep Bahal and others added 5 commits November 9, 2020 17:08
* Add all LasPoint types
* Added tests
* Added github actions for CI/CD
* Added laszip connection
* Updated Readme.md
* reset github actions
* PR changes
@c42f
Copy link
Collaborator

c42f commented Nov 23, 2020

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)

@msbahal msbahal force-pushed the Las1_4SupportWithAllPointFormats branch 4 times, most recently from 1c9476c to c69e05f Compare November 23, 2020 06:35
@msbahal msbahal force-pushed the Las1_4SupportWithAllPointFormats branch from c69e05f to 71a2641 Compare November 23, 2020 06:45
@msbahal
Copy link
Author

msbahal commented Nov 23, 2020

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
Copy link
Owner

visr commented Nov 23, 2020

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
Copy link
Collaborator

evetion commented Nov 23, 2020

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.

visr added a commit to visr/Yggdrasil that referenced this pull request Nov 28, 2020
Which is used to pipe Laz files in LasIO.jl, and is needed for visr/LasIO.jl#37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants