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 #12303 #12322

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

Fixes issue #12303 #12322

wants to merge 2 commits into from

Conversation

jfayot
Copy link
Contributor

@jfayot jfayot commented Nov 21, 2024

Description

This PR fixes a regression from #12230 (Fix allowPartial parameter handling in getBoundingSphere to prevent blocking by other visualizers in update function).

Issue number and link

Fixes issue #12303 (TypeError on setting the Viewer selectedEntity)

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

Thank you for the pull request, @jfayot!

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

@jfayot
Copy link
Contributor Author

jfayot commented Nov 21, 2024

To be honest, I'm not really confident with this PR!
My feeling is that #12230 should be reverted until a stable fix is found for #4647.
Moreover, if you look closely at this sandcastle you will notice that the model entity is actually not flickering anymore, but the polyline is still flickering!

@jfayot
Copy link
Contributor Author

jfayot commented Nov 21, 2024

Going further, in the original forum post, @marco13 suggests to use CallbackProperty which I think too is the right approach as demonstrated in this sandcastle (working with and without #12230 PR and where you can notice that neither the model entity nor the line entity are flickering).

In this same post, @Levi-Montgomery argues that CallbackProperty would not be suited for polylines with ~100000 points for performance reasons, which is true, but for such large polylines I would not use entities anymore, but rather the GeometrryInstance approach

@Levi-Montgomery
Copy link

Levi-Montgomery commented Nov 21, 2024

The Polyline flickering is separate from the model flickering, the Polyline flicker is just it being deleted and redrawn when you update its position points. You'll notice it also still flickers in the czml sandcastle as well. Might be a better way to do that but was out of scope.

The bug my MR fixed was a brief camera detachment from the tracked entity, the position of the entity updated but the camera position update was being blocked by the polyline's creation of the bounding sphere. The user would observe this as the model flickering. And my fix was complete for this bug.

Evidently based on what I am reading there were consequences to my fix that did not cause a test case to fail and went undetected in mine and Luke's testing. I understand if the correct course of action is to revert my MR pending a proper fix, as your bug is more serious.

My guess on a proper fix would be that the initial call to to .getBoundingSphere call i modified in my MR allowPartial should be false, and only subsequent calls should allowPartial be true.

@ggetz
Copy link
Contributor

ggetz commented Nov 21, 2024

My feeling is that #12230 should be reverted until a stable fix is found for #4647.

@jfayot Based on @Levi-Montgomery's comments, what is your recommendation for moving forward?

@jfayot
Copy link
Contributor Author

jfayot commented Nov 21, 2024

@ggetz I don’t have a clear-cut opinion on this. May be you or someone from the Cesium team could have a look at it?

@Levi-Montgomery
Copy link

@ggetz My testing seems that this MR we are in fixes jfayot's issue.

I would recommend a cesium dev look this MR over and decide if the fix is acceptable. If not, revert my #12230 MR and add a test case for jfayot's error so it doesn't inadvertently get broken again, then open an issue for another fix.

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.

4 participants