-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 updateBatchTableBoundingSpheres #4461
Conversation
Just waiting on unit tests. @mramato let me know when this is ready =) |
#4428 is fixed, but I'm still getting a crash with the KML file included in #4431
|
We were previously only applying if a display condition existed.
I'm still working on this, so there's a chance that I'll fix it or it's a completely different unrelated crash. |
alright, let me know when this is ready then |
This is ready. I'm going to open a PR for #4431 separately since fixing the first issue exposes a new one. |
@pjcozzi can you take a quick look at this and merge it? It fixes the problem, I'm just not 100% sure on the code changes. |
batchTable.setBatchedAttribute(i, center2DLowIndex, encodedCenter.low); | ||
|
||
batchTable.setBatchedAttribute(i, radiusIndex, radius); | ||
var modelMatrix = defaultValue(primitive.modelMatrix, Matrix4.IDENTITY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
var modelMatrix = primitive.modelMatrix;
if (defined(modelMatrix)) {
boundingSphere = BoundingSphere.transform(boundingSphere, modelMatrix, boundingSphere);
}
Just that comment. Also, update CHANGES.md |
Also clean up model modelMatrix check.
Thanks @pjcozzi, good call. Both are done. |
Yes, we discussed this earlier in the PR: #4461 (comment) |
Sounds good. |
updateBatchTableBoundingSpheres was using the BoundingSphere center without applying the modelMatrix to the sphere first.
Fixes #4431 and #4428
@bagnell as discussed offline, this may not be the most correct fix, but let me know what you think. We probably want a unit test for this as well.