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

Added support for large Rectangles in OrientedBoundingBox.fromRectangle #8475

Merged
merged 8 commits into from
Dec 20, 2019

Conversation

IanLilleyT
Copy link
Contributor

@IanLilleyT IanLilleyT commented Dec 18, 2019

There have been reports of 3D tiles that use TileBoundingRegion disappearing when viewing from a horizon angle. This is because the root tile generates a broken OrientedBoundingBox when the Rectangle's width is greater than half the ellipsoid. The terrain system doesn't have this problem because it handles this edge case and generates a BoundingSphere instead.

In this PR OrientedBoundingBox.fromRectangle has been made to work with larger rectangles. The approach is different than small rectangles which orient the OBB around the rectangle's center normal. I tried doing it this way for large rectangles but it produced very poorly fitted, albeit correct OBBs. The final approach ignores the latitude when calculating the OBBs orientation.

With this fix, all of the special case code is gone. I did a good amount of testing but this change impacts a lot of core systems so more eyes on it the better.

Fixes #4483

Sandcastle

Examples:
Screen Shot 2019-12-18 at 5 46 45 PM
Screen Shot 2019-12-18 at 5 45 45 PM
Screen Shot 2019-12-18 at 5 45 33 PM
Screen Shot 2019-12-18 at 5 44 46 PM

3D tile before (incorrectly culled):
Screen Shot 2019-12-18 at 5 53 26 PM
3D tile after:
Screen Shot 2019-12-18 at 5 51 35 PM

@loshjawrence
@lilleyse
@jasonbeverage

@cesium-concierge
Copy link

Thanks for the pull request @IanLilleyT!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@IanLilleyT IanLilleyT changed the title Added support for large rectangle to OrientedBoundingBox.fromRectangle Added support for large Rectangles in OrientedBoundingBox.fromRectangle Dec 18, 2019
@jasonbeverage
Copy link
Contributor

Very cool, this will help tremendously with global datasets!

@hpinkos
Copy link
Contributor

hpinkos commented Dec 19, 2019

@IanLilleyT could this fix #4483?

@IanLilleyT
Copy link
Contributor Author

IanLilleyT commented Dec 19, 2019

@hpinkos Just tested and it does not fix it, but I'm looking into it

@IanLilleyT
Copy link
Contributor Author

@hpinkos Looks like GroundPrimitive swapped the order of the minimumHeight and maximumHeight parameters to OrientedBoundingBox.fromRectangle. My newest commit fixes it.

Sandcastle from #4483

@hpinkos
Copy link
Contributor

hpinkos commented Dec 19, 2019

GroundPrimitive swapped the order of the minimumHeight and maximumHeight parameters

🤦‍♀️

Nice catch @IanLilleyT 😆

@IanLilleyT
Copy link
Contributor Author

@lilleyse fixed some bugs, ready for review

@lilleyse
Copy link
Contributor

Looks good based on my checks

  • The number of render tiles is the same between this branch and master. I tested all the different terrain providers and 3D Tiles.
  • The OBBs look good with rectangles spanning from positive longitude to negative longitude (crossing IDL)
  • Confirmed that the Alaska tileset above and another tileset that had this problem are now working in 3D.

The only thing to fix now is there's a crash when switching to 2D or CV with the Alasaka tileset.

crash

@hpinkos
Copy link
Contributor

hpinkos commented Dec 19, 2019

@lilleyse did you check rectangles at the poles?

@lilleyse
Copy link
Contributor

@hpinkos yes I forgot to mention that, I checked as many extreme cases as I could in the Sandcastle example including poles.

This is how I ended up hitting this bug, which also happens in master: #8478

@IanLilleyT
Copy link
Contributor Author

@lilleyse That crash you posted is a different problem which is fixed here: #8482

@lilleyse
Copy link
Contributor

@IanLilleyT cool, I'll check that out next

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.

GroundPrimitive bounding volume doesn't take curvature of the earth into consideration
5 participants