visr / LasIO.jl

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

Roadmap for v0.1 #4

Open JoshChristie opened 6 years ago

JoshChristie commented 6 years ago

Hey,

Thanks for this package!

Was the plan for this package to get registered? What needs to be done to get there?

Thanks,

Josh

visr commented 6 years ago

I think it's a good idea to register this package. Before I do I'd like to go over the API once more, I may get to this next week. If you'd like to see any changes, let me know.

@c42f, your cjf/las14 branch, shall we integrate it before registering? I'd be for supporting 1.4, though don't have experience with dealing with them yet.

Also, I'm using the laszip branch now to pipe through laszip for laz reading/writing. Do you think this belongs in master? I like the package pure julia right now, or could we just add the functions and not provide laszip ourselves? A pure julia laszip implementation would be cool, but quite a project I think.

c42f commented 6 years ago

I'm really happy to have this package, but the API, I think, could definitely be refined a bit. Perhaps it's worth considering a simultaneous first release of:

As to the state of the las14 branch, I'm afraid there's quite a bit of work to get that into shape before we can use it properly. When I started writing the branch, I found myself doing quite a bit of refactoring to generalize the API into a shape which seemed sensible. I didn't finish though before other work intervened.

Thinking back, here's some things which I was thinking about:

To some extent these are cosmetic things. API-wise, I guess there's two themes: 1) Higher level access to the header, both to generalize between las versions, and to allow the user to construct a header in a more robust way, which doesn't require knowing the details of the las spec for each las version. 2) Have an abstraction where you can treat the array of las points independently from the las header, and still extract meaningful coordinates.

visr commented 6 years ago

Thanks a lot Chris for your thoughts. I think pretty much all your points can be converted into checkboxes :smile:. I'll try to get v0.0.1 registered in the short term. What do you think about merging the laszip branch for v0.0.1 as well? Any additional thoughts about that?

It might be useful to have low level raw LasPoint types, without the fixed point encoding for the colors. I've seen people use the color channels for other things, and treat them as integers (alas), so sometimes easy access to the raw integer values is useful.

I'm not sure about this, the binary representation currently directly corresponds to the spec, that's pretty raw already. The red::N0f16 is created by reinterpret(N0f16, read(io, UInt16)), if people want to use these fields differently they can just reinterpret it back right? Or we could provide constructors that accept UInt16.

c42f commented 6 years ago

What do you think about merging the laszip branch for v0.0.1 as well

Am I correct in thinking you're just piping through an external laszip binary for this, and there's no additional hard dependencies? In that case, it sounds perfectly reasonable and very useful to merge this. My only reason to suggest 0.0.1 was for backward compatibility with the current somewhat stable API, if we're going to do a big API rethink.

I'm not sure about this, the binary representation currently directly corresponds to the spec

Yes, it's certainly a good thing to conform to the spec by default. To be clear, I was suggesting having both a low level and a high level point type for each point format, with people only seeing the high level by default. But I think you're right that we should just conform to the spec for now, and let people use reinterpret if they want to do something nasty.

(I have observed various las files in the wild which don't really conform to the suggested encoding (for example, treating 255 as the maximum value, as permitted by the 1.1 spec which doesn't have an official stance IIRC). I've also observed people abusing las color fields for other data. It's unfortunate, but these are real files which we sometimes have to deal with!)

visr commented 6 years ago

there's no additional hard dependencies? Correct. I'll polish it up and submit a PR. It doesn't break the master API, only extends it to LAZ.

To have both high level and low level interfaces sounds right to me, I indeed don't want to force users to learn the spec.

c42f commented 6 years ago

Oh by the way, thanks for taking all that in. It ended up being quite a wall of text!

visr commented 6 years ago

Here is the PR for v0.0.1, with the existing API: https://github.com/JuliaLang/METADATA.jl/pull/10831

c42f commented 6 years ago

Nice, thanks a lot :-)

visr commented 6 years ago

It's merged now. I'll leave this issue open as a roadmap for v0.1.