-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Fixes issue #12465 (Entity Tracking broken) #12467
base: main
Are you sure you want to change the base?
Conversation
Thank you for the pull request, @jfayot! ✅ We can confirm we have a CLA on file for you. |
/** | ||
* The bounding sphere of the object. | ||
* @type {BoundingSphere} | ||
*/ | ||
this.boundingSphere = undefined; |
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.
Can you explain why this was removed? More than it just works now? The correlation between this and the scratch value and the entities actual bounding sphere all still feels a little funky to me.
If we do still want to remove this we will need to go through the deprecation process as this is part of the public API and people may be relying on it.
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.
I have no answer for you @jjspace.
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.
@jjspace I recognize that I have no idea why "it just works now!"
Even this bounding sphere parameter inside EntityView.update
looks weird: why would you offer this as a parameter when there is no way to get a bounding sphere from an Entity
in the public API?
In #3954, the bounding sphere was moved from EntityView
constructor to update
method, but again, why would you impose to provide a bounding sphere to EntityView
(which is related to the high level Entity API) when you don't have the ability to get this bounding sphere from Entity's high level API?
Before #3954, the bounding sphere parameter in the constructor was documented as:
@param {BoundingSphere} [boundingSphere] An initial bounding sphere for setting the default view.
This "initial" aspect has completely disappeared. My guess is that it should be restored, AND that the Entity's bounding sphere should be computed from inside the EntityView
implementation and should disappear from the public API.
Otherwise provide a public Entity.computeBoundingSphere
method...
Description
This is an attempt to fix broken entity tracking when viewFrom is set:
Restored
boundingSphereScratch
instead ofentityView.boundingSphere
insideCesiumWidget._onTick
.Issue number and link
Fixes #12465
Testing plan
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change