10957 zoom to entities without globe fix

[fullstacc]

Fixes #10957 .

• Added 2 lines which check for the presence of a globe & terrain provider and adds a substitute ellipsoid if globe doesn't exist.
• Based on the description for the issue, I looked at the other visualizers and didn't see this needed in any of the others

The behavior noted from the issue is fixed

[cesium-concierge]

Thanks for the pull request @fullstacc!

• ❌ Missing CLA.
  • Please send in a Contributor License Agreement (CLA) and comment back here to let us know to check this!
• ❔ 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.

[fullstacc]

CLA submitted

[ggetz]

Instead of assuming the ellipsoid value, I think it would actually be better to return early and skip some of these unneeded calculations below when globe is undefined.

I believe when the globe is undefined, we can simply skip the hasHeightReference block entirely as there is no ellipsoid to offset the position against, and we can just use the position as-is.

[fullstacc]

thanks for the feedback! I added a condition to hasHeightReference that skips that block if the globe is undefined. I also removed the ellipsoid being instantiated as Ellipsoid.WGS84 if one isn't present, and moved the ellipsoid being defined to within the hasHeightReference block as you would need a globe for this anyway

[ggetz]

Thanks @fullstacc! This will be a helpful fix for sure!

After we determine the best approach, it would be appreciated if you could add:

1. A summary of the fix to CHANGES.md
2. A unit test to make sure the issue doesn't reoccur in the future

Let us know if you need any guidance on those.

[fullstacc]

@ggetz , my apologies in advance: How should I structure my inputs to your CHANGES.md?
looking at the current file, I can see the most recent change states "1.105 - 2023-05-01" at the top. I thought this might be a non-US style of stating dates, but then I noticed right underneath that is "1.104 - 2023-04-03".

Should I add a subheading between these two dated "2023-04-13" with my fixes listed?
Also, would this be 1.106?

[ggetz]

my apologies in advance: How should I structure my inputs to your CHANGES.md?

No worries! The next release will be 1.105 on May 1, 2023 (The release, 1.104, went out April 3, 2023).

We should include all new changes under the 1.105 - 2023-05-01 heading until the release goes out. Does that make sense?

[fullstacc]

hi @ggetz ! I'm revisiting this after repeated tries last week. It looks like the CI build fails due to an issue importing the Viewer in the createViewer file. I imported and leveraged createViewer to test my modelvisualizer changes, but I didn't modify the createViewer file.

Should I not be using this createViewer for my tests?

[ggetz]

Hi @fullstacc, createViewer depends on widget code, which is a higher level of abstraction than the visualizer code. So really, any test calling viewer.zoomTo belong in ViewerSpec.

To be more explicit about the architecture, and to keep this from happening in the future, I moved createViewer to packages/widgets/Specs.

Additionally, zoomTo returns a promise which needs to be awaited, otherwise side effects will occur.

Finally, we use the defined function to check for both undefined and null.

I pushed a fix with the above changes.