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 8074 #8081

Merged
merged 3 commits into from
Aug 23, 2019
Merged

Fix 8074 #8081

merged 3 commits into from
Aug 23, 2019

Conversation

loshjawrence
Copy link
Contributor

An example fix for #8074.

Right now there are .show s on individual primitives but there's also the scene's primitives.show.

This fix treats the scene's primitive.show like a master switch and would be the simplest fix for this.

localhost sandcastle

@cesium-concierge
Copy link

Thanks for the pull request @loshjawrence!

  • ✔️ 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.

@mramato
Copy link
Contributor

mramato commented Aug 19, 2019

This fix treats the scene's primitive.show like a master switch and would be the simplest fix for this.

Treating it like a master switch instead of having it toggle off the individual primitives is what we do elsewhere. So I assume that means this is the "right" fix?

@mramato
Copy link
Contributor

mramato commented Aug 19, 2019

@OmarShehata can you confirm everything is good on your end?

@lilleyse will you have time to look at this soon?

@OmarShehata
Copy link
Contributor

I can confirm this fixes the original issue, but you can still get this effect to happen by using tileset.show instead of scene.primitives.show. See this localhost Sandcastle as brought up here #8074 (comment).

@loshjawrence
Copy link
Contributor Author

loshjawrence commented Aug 19, 2019

@OmarShehata I added @dennisadams other fix for this

@mramato
Copy link
Contributor

mramato commented Aug 23, 2019

Where are we at here. @OmarShehata please merge if this is ready so it can sit in master before the release to make sure it fully fixes the problem. CC @lilleyse any thoughts

@lilleyse
Copy link
Contributor

Looks fine to me.

@mramato
Copy link
Contributor

mramato commented Aug 23, 2019

@OmarShehata please merge once you confirm the issue you could still reproduced is fixed. Thanks!

@OmarShehata OmarShehata merged commit 303fced into master Aug 23, 2019
@OmarShehata OmarShehata deleted the bugfix-8074 branch August 23, 2019 19:20
@OmarShehata
Copy link
Contributor

Works great, thanks @loshjawrence !

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.

5 participants