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

Fixes issue #12465 (Entity Tracking broken) #12467

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jfayot
Copy link
Contributor

@jfayot jfayot commented Feb 7, 2025

Description

This is an attempt to fix broken entity tracking when viewFrom is set:
Restored boundingSphereScratch instead of entityView.boundingSphere inside CesiumWidget._onTick.

Issue number and link

Fixes #12465

Testing plan

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

github-actions bot commented Feb 7, 2025

Thank you for the pull request, @jfayot!

✅ We can confirm we have a CLA on file for you.

@ggetz ggetz requested a review from jjspace February 10, 2025 19:26
@ggetz
Copy link
Contributor

ggetz commented Feb 10, 2025

Thank you for the fix @jfayot!

@jjspace Could you please review?

Comment on lines -349 to -353
/**
* The bounding sphere of the object.
* @type {BoundingSphere}
*/
this.boundingSphere = undefined;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entity Tracking sandcastle broken in 1.126
3 participants