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

Fix: bound calculation for negative values #77

Merged
merged 1 commit into from
May 13, 2024

Conversation

wischi-chr
Copy link
Contributor

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.

  • Maybe the new inverse_internal method into transform.rs?

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.

Copy link
Owner

@gadomski gadomski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks so much for the PR!

Maybe the new inverse_internal method into transform.rs?

I agree with this, let's keep the method definitions all in that file.

Co-authored-by: Pete Gadomski <pete.gadomski@gmail.com>
@gadomski gadomski enabled auto-merge (rebase) May 13, 2024 12:06
@gadomski gadomski merged commit 0ab652d into gadomski:main May 13, 2024
3 checks passed
@gadomski
Copy link
Owner

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

@wischi-chr wischi-chr deleted the fix-bounds-calculations branch May 19, 2024 19:02
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

2 participants