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 signed distance to bounding sphere. #6452

Merged
merged 2 commits into from
Apr 18, 2018
Merged

Fix signed distance to bounding sphere. #6452

merged 2 commits into from
Apr 18, 2018

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Apr 16, 2018

Fixes #6451.

CC @emackey

@cesium-concierge
Copy link

Signed CLA is on file.

@bagnell, thanks for the pull request! Maintainers, we have a signed CLA from @bagnell, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

Copy link
Contributor

@emackey emackey left a comment

Choose a reason for hiding this comment

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

Confirmed fixed. Thanks!

@emackey
Copy link
Contributor

emackey commented Apr 16, 2018

Will wait for Travis...

@emackey
Copy link
Contributor

emackey commented Apr 16, 2018

Hmm. Are there test failures here?

@bagnell
Copy link
Contributor Author

bagnell commented Apr 16, 2018

Looks like it. I couldn't reproduce it locally though.

@emackey
Copy link
Contributor

emackey commented Apr 17, 2018

Scene/LabelCollection computes bounding sphere in 3D
Expected 111158688.34332076 to equal 4862789.037706432.

I was able to see it in Jasmine locally when running the whole test suite, but not running LabelCollectionSpec in isolation. It could be some bad interaction with other tests?

It's too much of a red flag that this is a bounding-sphere related test fail, and I don't see it in master or other recent PRs. I think it's local to this branch, unfortunately.

@emackey
Copy link
Contributor

emackey commented Apr 17, 2018

Also Travis didn't get the same result as Jasmine. Here's Travis:

Expected 107984155.04338884 to equal 4862789.037706432.

@mramato
Copy link
Contributor

mramato commented Apr 17, 2018

Just to confirm, I can reproduce the failure locally every time with:

npm run test -- --browsers Electron --webgl-stub --failTaskOnError --suppressPassed

it does not happen at all for me in master

@bagnell
Copy link
Contributor Author

bagnell commented Apr 18, 2018

I reverted the changes to what was in 1.44. The distance to bounding sphere wasn't needed for log depth but I noticed that it was returning a size for volumes behind the camera.

@emackey
Copy link
Contributor

emackey commented Apr 18, 2018

Looks good, tests pass. Thanks @bagnell.

@emackey emackey merged commit 7307f81 into master Apr 18, 2018
@emackey emackey deleted the distance-to-bv branch April 18, 2018 21:26
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.

4 participants