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 globe baselayer error #12274

Merged
merged 10 commits into from
Nov 15, 2024
Merged

Conversation

Masty88
Copy link
Contributor

@Masty88 Masty88 commented Oct 31, 2024

Description

This PR adds a check to prevent using baseLayer when globe is disabled, which would cause render errors.
The check throws a DeveloperError with a clear message to help developers understand and fix the issue.

The fix:

  • Adds validation in the Viewer constructor to check for invalid baseLayer usage
  • Throws DeveloperError with clear message when attempting to use baseLayer with globe: false
  • Allows valid cases like baseLayer: false or when globe is not explicitly disabled

Issue number and link

Fixes #12244

Testing plan

Added two new test cases in ViewerSpec.js:

  1. Verifies error is thrown when baseLayer is provided with globe: false
  2. Verifies no error when globe is false but baseLayer is not provided

Additional testing:

  • Verified existing tests continue to pass
  • Manually tested in Sandcastle to ensure valid configurations still work
  • Verified error message is clear and helpful

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with "Fixed error when using baseLayer with globe disabled"
  • I have added unit tests to ensure consistent code coverage
  • I have updated the inline documentation
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @Masty88!

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

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 for the PR @Masty88!

packages/widgets/Source/Viewer/Viewer.js Show resolved Hide resolved
@ggetz
Copy link
Contributor

ggetz commented Nov 15, 2024

Looks goods @Masty88! I just made a small tweak to move your CHANGES.md item to the relevant release.

Once CI passes, this should be good to merge. Thanks!

@ggetz ggetz merged commit 74663e9 into CesiumGS:main Nov 15, 2024
4 checks passed
@Masty88
Copy link
Contributor Author

Masty88 commented Nov 16, 2024

Perfect thank you!

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.

Using baseLayer with globe: false results in breaking error
2 participants