Wrong assessment of terrain tile visibility

[dennisadams]

Bumped into this piece of code while dealing with #9032 :

cesium/Source/Scene/GlobeSurfaceTileProvider.js

Lines 578 to 585 in 44ac2ff

	// Terrain state changed. If:
	// a) The tile is visible, and
	// b) The bounding volume is accurate (updated as a side effect of computing visibility)
	// Then we'll load imagery, too.
	if (
	this.computeTileVisibility(tile, frameState, this.quadtree.occluders) &&
	surfaceTile.boundingVolumeSourceTile === tile
	) {

This seems like a simple oversight, expecting computeTileVisibility to return a boolean. But computeTileVisibility actually returns a Visibility value where Visibility == {NONE: -1, PARTIAL: 0, FULL: 1}. This leads to cases where partially visible tiles pass as non-visible (since 0 evaluates as false), and where non-visible tiles pass as visible (since -1 evaluates as true).

I assume it's supposed to check if the Visibility return value is not Visibility.NONE, just like here:

cesium/Source/Scene/QuadtreePrimitive.js

Lines 1191 to 1194 in 44ac2ff

	if (
	tileProvider.computeTileVisibility(tile, frameState, occluders) !==
	Visibility.NONE
	) {

On a sidenote, although the Visibility.js and Intersect.js objects are quite equivalent, it may be clearer to always return a Visibility value from computeTileVisibility and not an Intersect value like here for instance:

cesium/Source/Scene/GlobeSurfaceTileProvider.js

Line 742 in 44ac2ff

return Intersect.INTERSECTING;

[mramato]

Thanks @dennisadams this looks like a pretty obvious bug. I wonder how long it's been there (git blame is a little useless because of format-only changes that went in a while ago).

@lilleyse are we missing something here or is this just an oversite?

[lilleyse]

It looks like it's been there since #7061. Sorry to keep passing this along, but @kring could you check this?

[kring]

Good catch @dennisadams! This is definitely a bug, and it will cause a one-frame delay in loading imagery sometimes, and load imagery that isn't needed at all other times. The cases where either occur should be relatively rare (which is why we didn't notice before), but they'll happen. Are you up for opening a pull request to fix this?

[dennisadams]

Sure