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 allowPartial parameter handling in getBoundingSphere to prevent blocking by other visualizers in update function #12230

Merged
merged 5 commits into from
Oct 15, 2024

Conversation

Levi-Montgomery
Copy link

@Levi-Montgomery Levi-Montgomery commented Oct 2, 2024

Description

This pull request addresses a bug in the getBoundingSphere method where the allowPartial parameter was not functioning correctly. The issue caused the bounding sphere creation for one entity to be blocked by another entity belonging to a different visualizer that is not ready, even when allowPartial was set to true. This, combined with the fact that the allowPartial parameter being set to false when creating a boundingSphere for the tracked entity, resulted in the flicker bug seen in issue Issue 4647 and Community Forum Post 35516. The tracked entity was unable to get its bounding sphere if the Polyline was being drawn at the same time.

Changes Made:
Added a check for !allowPartial before setting the BoundingSphereState to PENDING in the getBoundingSphere method.

Tested:
Ensured that the method does not halt and return pending if allowPartial is true even when the desired visualizer is ready.
Tested with Cesium Sandcastle Provided in Community Forum Post 35516
(For some reason i cant get the 2016 issue sandcastle working locally on my mac even when im on the main branch so ive been unable to test it with that)

Issue number and link

4647 #4647
35516 https://community.cesium.com/t/how-to-fix-flicker-upon-updating-position-of-an-entity/35516

Testing plan

I used the example cesium sandcastle code provided in the community forum post and visually verified the flicker is no longer there.

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 Oct 2, 2024

Thank you for the pull request, @Levi-Montgomery!

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

@ggetz
Copy link
Contributor

ggetz commented Oct 3, 2024

Thanks for the fix @Levi-Montgomery! @lukemckinstry could you please do a review pass on this?

@Levi-Montgomery
Copy link
Author

@lukemckinstry Thanks for testing.

the model is no longer visible.

Could it be because the model path on line 31 does not exist in that location on your computer when you're running locally?

I did have to change that string to a different 3D model that i already had on my computer for testing purposes.

@Levi-Montgomery
Copy link
Author

@lukemckinstry Ah, I see. If you replace the path on line 31 with ../../SampleData/models/CesiumDrone/CesiumDrone.glb it will work. Not sure why a single ../ works in the deployed environment but in local you need ../../

@lukemckinstry
Copy link
Contributor

Thanks for this PR @Levi-Montgomery and for helping me troubleshoot.
The flickering caused by the misalignment of the model and camera positions appears fixed in the sandcastle example described in #4647 and the sandcastle example at the top of the forum thread https://community.cesium.com/t/how-to-fix-flicker-upon-updating-position-of-an-entity/35516
This is looking good @ggetz

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @Levi-Montgomery!

Please update CHANGES.md with a summary of your fix. Then this should be good to merge,

@Levi-Montgomery
Copy link
Author

@ggetz Awesome!! I've pushed up a summary.

@ggetz ggetz merged commit 0e9a425 into CesiumGS:main Oct 15, 2024
4 checks passed
@javagl
Copy link
Contributor

javagl commented Oct 18, 2024

I'd have to cross-check the details, but maybe someone can quickly say whether this fixes #4647 (meaning that this issue could be closed) ... ?

@ggetz
Copy link
Contributor

ggetz commented Oct 18, 2024

Thanks @javagl! Closed.

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