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

Ensure a properly scaled bounding box #40

Merged
merged 2 commits into from
Oct 29, 2021
Merged

Ensure a properly scaled bounding box #40

merged 2 commits into from
Oct 29, 2021

Conversation

Christopher22
Copy link
Contributor

First of all, thank you for the amazing work, @gadomski ! Currently, the bounding box of the LAS file may be invalid in some pathologic cases. Consider the points specified in the csv file attached. If the included coordinates are serialized with the default Transform and the resulting file is checked with lasinfo, the software will warn about multiple issues:

lasinfo (210720) report for 'D:\pathologic.las'
[...]
scale factor x y z:         0.001 0.001 0.001
  offset x y z:               0 0 0
  min x y z:                  -2.787 -0.932 -5.806
  max x y z:                  0.609 1.543 -0.094
WARNING: stored resolution of min_x not compatible with x_offset and x_scale_factor: -2.786861896514893
WARNING: stored resolution of min_y not compatible with y_offset and y_scale_factor: -0.932222902774811
WARNING: stored resolution of min_z not compatible with z_offset and z_scale_factor: -5.806345939636231
WARNING: stored resolution of max_x not compatible with x_offset and x_scale_factor: 0.60914021730423
WARNING: stored resolution of max_y not compatible with y_offset and y_scale_factor: 1.542856812477112
WARNING: stored resolution of max_z not compatible with z_offset and z_scale_factor: -0.094414718449116
[...]
[pathological.csv](https://github.com/gadomski/las-rs/files/7442998/pathological.csv)
WARNING: 1 points outside of header bounding box
[...]
WARNING: real max z larger than header max z by 0.000415

While the first warnings regarding the scaling of the bounding box may be neglectable the point outside of the bounding box is it clearly not. For example, it will seriously hinder the visualisation with potree.

This patch scales the bounding box accordingly to the transform, too. As a direct implication there are no longer any problems with the bounding box and floating-point issues.

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.

LGTM. Can you rebase onto latest main to pick up the CI fix in 78a2b74, and we should be good go. Appreciate the fix!

@gadomski gadomski merged commit 61ef7c1 into gadomski:main Oct 29, 2021
@Christopher22 Christopher22 deleted the fix_bounding_box_floating_point_issues branch October 29, 2021 16:25
@Christopher22
Copy link
Contributor Author

Amazing, thanks for review! May you upload a new version with the bugfix to crates.io? Such semantic version are so much cleaner than the direct git dependencies in a cargo.toml...

@gadomski
Copy link
Owner

Done: https://crates.io/crates/las/0.7.5.

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