Render globe tiles if any layer is loaded

[sfariaNG]

Updated globe tiles to render imagery if any available image layer has finished loading.
Fixes #7974

[cesium-concierge]

Thank you so much for the pull request @sfariaNG! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

• ❌ Missing CONTRIBUTORS.md entry.
  • Please add yourself to the contributors file!
• ❌ 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.
• Works (or fails gracefully) in IE11.

[sfariaNG]

Guidelines addressed. Not sure a new unit test is necessary for this fix since it's so small, but let me know if you think otherwise. This should be ready.

[lilleyse]

@sfariaNG just checking - did you fill out a CLA?

[lilleyse]

@sfariaNG just checking - did you fill out a CLA?

Never mind.

[tfili]

@kring Do you have any input here? I’m not sure if there are any implications here that I’m missing.

[nmschulte-aviture]

No more need for isRenderable variable; this reads more cleanly as tile.renderable &= isAnyTileLoaded (or tile.renderable = tile.renderable && isAnyTileLoaded).

[sfariaNG]

That's true. I would lean toward the latter option to maintain the boolean.

[mortac8]

Is there anything else that needs to be done for this? We are currently including it manually in our Cesium builds.

[OmarShehata]

@mortac8 I think we just haven't had a reviewer dedicate time to looking into this - hopefully we can get it looked at before the next release.

[likangning93]

@sfariaNG I grabbed a copy of the branch and merged in master, for some reason it looks like apps that load terrain on startup like the "Cesium Viewer" checkbox item above (https://cesiumjs.org/Cesium/Apps/CesiumViewer/index.html at https://cesiumjs.org/Cesium) don't render the globe until the imagery layers have been changed. Can you confirm?

Interestingly enough, if I put a breakpoint anywhere in GlobeSurfaceTile.prototype.processImagery I can get imagery to load, so I wonder if requests are getting cancelled due to overload somehow.

Looking at the network tab in a Sandcastle with just Cesium world terrain and Natural Earth imagery, it looks like a couple requests for both imagery and terrain are still getting through. Here's what I tested with specifically:

var viewer = new Cesium.Viewer('cesiumContainer', {
    imageryProvider : Cesium.createTileMapServiceImageryProvider({
        url : Cesium.buildModuleUrl('Assets/Textures/NaturalEarthII')
    }),
    terrainProvider: Cesium.createWorldTerrain()
});

If it helps, I also tried logging out some tile state at the end of GlobeSurfaceTile.prototype.processImagery:

console.log(tile.x + ' ' + tile.y + ' renderable: ' + tile.renderable + ' isDoneLoading: ' + isDoneLoading + ' isAnyTileLoaded: ' + isAnyTileLoaded);

Here it kind of looks like isAnyTileLoaded doesn't become true even though isDoneLoading becomes true:

[likangning93]

I'm seeing something similar in IE11 and Chrome 77 on Windows 10, with the difference that I don't seem to need Cesium World Terrain to achieve the effect, just the default Bing maps imagery layer.

[likangning93]

I'm seeing something similar in IE11 and Chrome 77 on Windows 10, with the difference that I don't seem to need Cesium World Terrain to achieve the effect, just the default Bing maps imagery layer.

Wait, no, this might just be due to an expired API key. I'm still seeing the same problems with Natural Earth imagery and terrain though.

[mortac8]

Random note: We have historically seen intermittent globe or imagery loading issues (since ~Cesium 1.4x) with or without safariaNG's change. We have a current ticket for it internally but have previously written it off to poor network performance or something in our code.

[OmarShehata]

I can confirm this fixes the original issue referenced! I can also confirm I see @likangning93 's bug.

@mortac8 this one isn't an intermittent issue though. If you try any Sandcastle that loads terrain on startup in this branch, like this one:

http://localhost:8080/Apps/Sandcastle/index.html?src=3D%20Tiles%20Photogrammetry.html

(You'll need to add in a token or merge the latest master branch)

You'll see it does not load any tiles. So this is going to break a lot of CesiumJS applications if merged as-is.

[mortac8]

What if you back out the change and make just one modification to GlobeSurfaceTile.js on master? This works for me (just changing the first && to an ||):

Line 340:
isRenderable = isRenderable || (thisTileDoneLoading || defined(tileImagery.readyImagery));

[sfariaNG]

I will take another look.

[sfariaNG]

Turns out just because layers are in the collection doesn't mean they are being loaded so my initial isAnyTileLoaded value wasn't always accurate. I've adjusted it to account for the case where no available layers are currently being loaded.

[sfariaNG]

This is ready for another look.

[sfariaNG]

Took another look today and realized the logic could be simplified (also helped by re-reading the comment from @likangning93 ). Ran it through 128 input combinations and got the same results as the original code except where expected (i.e. one layer loaded out of group).

CesiumTruthTableResults2.xlsx

[likangning93]

Works (or fails gracefully) in IE11.

Tested in another branch with master merged in for #8188

[likangning93]

@sfariaNG thanks for the updates, and for the extensive testing! And @mortac8 thanks for your input and suggestions.

This looks good on my end and works as advertised, @sfariaNG please just merge in master and resolve the conflict in CHANGES.md.

[sfariaNG]

I resolved the conflict using the UI and merged in master, though the CI seems to not like that. Let me know if I should rebase it.

[likangning93]

@sfariaNG I'm not sure what's going on with appveyor, but I think we're ok to just go ahead and merge. Thanks again!

[sfariaNG]

Thanks @likangning93 !