gadomski / las-rs

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

Fix: bound calculation for negative values #77

Closed wischi-chr closed 5 months ago

wischi-chr commented 5 months ago

Thank you for maintaining this awesome library.

I recently ran into a problem with the bounds. Another tool (PotreeConverter) crashed, because it found points outside the bounds.

At first I suspected some floating point issues, so I took a look at the source code where and how the bounds are calculated, just to realize that you already thought about that :-)

// bounds.rs

// Transform the bounds to be compatible with the chosen transform.
// Otherwise, points may lay outside of the bounding box due to floating-point issues.
pub fn adapt(&self, transform: &Vector<Transform>)

but to calculate the bounds both the upper bound (max) and the lower bound (min) are calculated by rounding to the nearest step (inside Transform.inverse) but to be correct the upper bound should be rounded up with ceil and the lower bound rounded down with floor.

I wrote some tests to confirm my suspicion (without touching the rest of the code) and indeed points are outside the bounds after calling the adapt method.

(
    Vector {
        x: 0.99999999,
        y: 0.99999999,
        z: 0.99999999,
    },
    Bounds {
        min: Vector {
            x: 1.0,
            y: 1.0,
            z: 1.0,
        },
        max: Vector {
            x: 1.0,
            y: 1.0,
            z: 1.0,
        },
    },
)

I implemented a fix in a backwards compatible way (I hope, I kindly ask you to review if I missed anything) but to do so I had to impl a new inverse_internal method on the Transform struct to allow the caller to determine the rounding mode. The new method and the RoundingMode enum are pub(crate) to not increase the public API surface.

After fixing the issue the case from above was calculated correctly and the tests I've written all passed. The external tool that originally crashed because of the bounds no longer complained.

(
    Vector {
        x: 0.99999999,
        y: 0.99999999,
        z: 0.99999999,
    },
    Bounds {
        min: Vector {
            x: 0.999,
            y: 0.999,
            z: 0.999,
        },
        max: Vector {
            x: 1.0,
            y: 1.0,
            z: 1.0,
        },
    },
)

Let me know if you need me to make some changes.

FYI I'm out of town for a few days, so it may take a few days (I guess next Monday) for me to respond.

gadomski commented 5 months ago

Awesome, thanks for the contribution! Released: https://crates.io/crates/las/0.8.7