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.

Sign up for GitHub

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

Jump to bottom

#8006 Fix incorrect billboard clamping when terrain provider changes #9079

Closed

bn-dignitas wants to merge 5 commits into CesiumGS:main from bn-dignitas:fix-duplicate-billboard-height-callbacks

Contributor

bn-dignitas commented Aug 3, 2020

#8006 Pass id of billboard to callback function to prevent multiple callbacks being registered for the same billboard in invalidateAllTiles, resulting in leaked/stale callbacks and bad clamped locations.

bn-dignitas added 2 commits

August 2, 2020 21:45


          CesiumGS#8006 Pass id of billboard to callback function to prevent mu…

0ba8738

…ltiple callbacks being registered for the same billboard in invalidateAllTiles, resulting in leaked/stale callbacks and bad clamped locations.


          Make sure callbacks with undefined owner are added to maintain behavi…

8bf60cc

…or for other primitive types

cesium-concierge commented Aug 3, 2020

Thanks for the pull request @bn-dignitas!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.
❔ Unit tests were not updated.
- Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

Contributor

hpinkos commented Aug 3, 2020

Thanks for the PR @bn-dignitas! I should have time to review this and test it out before the September release

bn-dignitas added 3 commits

August 11, 2020 09:57


          Fix billboard position issues when reused billboard height ref is cha…

749f7a5

…nged from clamped to none


          Store the billboard instead of the ID since billboards are reused and…

922488b

… IDs change


          Fix object type in documentation

e7497a7

cesium-concierge commented Sep 11, 2020

Thanks again for your contribution @bn-dignitas!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

6 similar comments

cesium-concierge commented Oct 11, 2020

Thanks again for your contribution @bn-dignitas!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge commented Nov 10, 2020

Thanks again for your contribution @bn-dignitas!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge commented Dec 10, 2020

Thanks again for your contribution @bn-dignitas!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge commented Jan 9, 2021

Thanks again for your contribution @bn-dignitas!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge commented Feb 8, 2021

Thanks again for your contribution @bn-dignitas!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge commented Mar 10, 2021

Thanks again for your contribution @bn-dignitas!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge commented Jun 8, 2021

Thanks again for your contribution @bn-dignitas!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

1 similar comment

cesium-concierge commented Sep 6, 2021

Thanks again for your contribution @bn-dignitas!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

ebogo1 added the category - billboards label

cesium-concierge commented Dec 5, 2021

Thanks again for your contribution @bn-dignitas!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

ggetz assigned ebogo1

ebogo1 reviewed

View reviewed changes

Source/Scene/QuadtreePrimitive.js

                 this._tilesInvalidated = true;
               };
+              function containsOwnerHeightCallbackData(heightCallbacks, owner) {
+                var found = false;

Contributor

ebogo1 Feb 10, 2022

Could this be changed to delete the found variable, replace found = true; break; with return true; inside the for-loop, and return false; at the end instead of return found?

ebogo1 reviewed

View reviewed changes

Source/Scene/Billboard.js

-                    owner._clampedPosition
-                  );
+                  // Billboards can be reused, i.e. by EntityCluster so check that the height

Contributor

ebogo1 Feb 10, 2022

For my understanding, is this also intended to fix the bug from your comment on the issue?

It looks like the QuadtreePrimitive customData list keeps growing and some stale height callbacks are being made with old coordinates.

I don't think the comment in the code is needed.

ebogo1 reviewed

View reviewed changes

Contributor

ebogo1 left a comment

Thanks @bn-dignitas! I confirmed the change fixes the billboard issue in the Sandcastle from #8006.

There are a few things left like a CHANGES.md entry and changing var to let and const but I think this is a good change.

Contributor

ggetz commented Feb 21, 2022

Due to inactivity, I'm going to close this PR to keep things tidy. @bn-dignitas if you get the chance to update this, please feel free to re-open. Thanks!

ggetz closed this

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

Labels

category - billboards