Skip to content

Fix incorrect computation of bounding boxes #841

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

Merged
merged 2 commits into from
May 11, 2022
Merged

Fix incorrect computation of bounding boxes #841

merged 2 commits into from
May 11, 2022

Conversation

kring
Copy link
Member

@kring kring commented May 9, 2022

Fixes #839

I "improved" the bounding box computation in #823. It looked surprisingly right considering how actually wrong it was. 😆
Generally it worked ok near the georeference origin, but got very bad for tiles that are not well aligned to the Unreal axes.

I think it's correct now, but it would definitely be wise for someone to double-check my math when reviewing this!

@cesium-concierge
Copy link

Thanks for the pull request @kring!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

@kring kring changed the title Fix incorrect computation of bound boxes Fix incorrect computation of bounding boxes May 9, 2022
@kring
Copy link
Member Author

kring commented May 9, 2022

After this is merged into ue5-main, post an update to https://community.cesium.com/t/linetrace-hit-detection-causing-value-to-be-world-origin/18478/3 to ask the user to check whether it fixes their problem as well.

FBoxSphereBounds result;
result.Origin = VecMath::createVector(center);
result.SphereRadius = sphereRadius;
result.BoxExtent = FVector(xs.GetAbsMax(), ys.GetAbsMax(), zs.GetAbsMax());
result.BoxExtent = FVector(sphereRadius, sphereRadius, sphereRadius);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's still a comment that says the sphere only has to reach the side of the box, is that still applicable / correct?

Also is all the stuff with the half-axes necessary in the BoundingSphere conversion, can't we just convert the sphere radius from meters to centimeters? Could we assume getTilesetToUnrealWorld is a rigid transform + a meters to centimeters scaling?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to avoid making any assumptions about the transformation, since it's just a parameter to the method. So instead I construct a bounding box around the sphere, transform the box, and then build a sphere inset in the box. The comment is referring to the fact that the sphere only needs to be inset in the box, rather than fully contain it. There could very well be a better way to do this, but I think it works reasonably well.

@nithinp7
Copy link
Contributor

nithinp7 commented May 10, 2022

@kring This seems to fix the issue and looks good to me! Only left one (probably unnecessary) comment, feel free to self-merge this otherwise.

@nithinp7 nithinp7 merged commit 32ee39f into ue4-main May 11, 2022
@nithinp7 nithinp7 deleted the boundingbox branch May 11, 2022 03:06
@kring kring added this to the Cesium for Unreal v1.13.2 milestone May 12, 2022
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.

3 participants