gadomski / las-rs

Read and write ASPRS las files, Rust edition.
MIT License
73 stars 32 forks source link

Quick and dirty hack to support AsyncReader #55

Closed bacek closed 7 months ago

bacek commented 1 year ago

This is not a fully fledged PR, just a discussion point.

I was going to use las-rs for web based LAS manipulations. Compile to WASM and extract metadata (and points in the future). But for proper support on javascript side all IO should be async.

Just checking is my approach somewhat reasonable to drive it to completion ready to merge. Or there is a better way of doing it?

(I also implemented AsyncRead for Reqwest, so I can actually do bytes range http requests to fetch parts of remote LAZ files without dowloading them in whole)

gadomski commented 1 year ago

Thanks for the contribution! Adding async support to las-rs would be a great addition -- with the advent of COPC, the ability to do range requests over the network will be even more critical.

While I don't have any numbers to back it up, doing an await on every single field read feels like overkill, e.g.: https://github.com/gadomski/las-rs/pull/55/files#diff-8297ae39a87a1e74cf75b7fe3b642d43539916224667129780ddf164bcf5e935R35-R38. My instinct is that a "chunked async" implementation may be more appropriate, where data are async read into buffers, and then those buffers are decoded into the las structures using the normal reader methods. This also would let use leverage the current decoding path without having to re-implement an async version.

Let me know if that doesn't make sense ... I'm just thinking about async for las-rs for the first time (this library predates the existence of the async keyword).

bacek commented 1 year ago
bacek commented 1 year ago

(For the record: last time I was working with rust was 3+ years ago, no async/await, no async io, just futures 0.1 and early stages of tokio)

Thanks for the contribution! Adding async support to las-rs would be a great addition -- with the advent of COPC, the ability to do range requests over the network will be even more critical.

Yes, this is what I'm hoping to achieve at the end of the day.

While I don't have any numbers to back it up, doing an await on every single field read feels like overkill, e.g.: https://github.com/gadomski/las-rs/pull/55/files#diff-8297ae39a87a1e74cf75b7fe3b642d43539916224667129780ddf164bcf5e935R35-R38. My instinct is that a "chunked async" implementation may be more appropriate, where data are async read into buffers, and then those buffers are decoded into the las structures using the normal reader methods. This also would let use leverage the current decoding path without having to re-implement an async version.

This makes total sense. We will need to expose some methods/traits like fn expected_buf_size() -> usize and fn parse_from(&buf: [u8]) -> Result<Self> inside raw crate. Than both sync and async version can use same parsing path for both of them. I'll try to create some minimal compiling example how it can look like. And the same approach will need to be applied to laz-rs.

Let me know if that doesn't make sense ... I'm just thinking about async for las-rs for the first time (this library predates the existence of the async keyword).

bacek commented 1 year ago

@gadomski I pushed d47dfb7 to explore how code can be structured to support both sync and async parsing. Just a rough idea.

gadomski commented 7 months ago

Closing as stale, please re-open with updates if there's more work on this PR.