setMightHaveLatentChildren(false) even if a tile has real children.#955
Merged
setMightHaveLatentChildren(false) even if a tile has real children.#955
Conversation
Member
Author
|
I'm merging this because it fixes a regression that would otherwise be in today's release. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes CesiumGS/cesium-unreal#1529
In #783, I renamed
shouldContinueUpdatingtomightHaveLatentChildrenin an attempt to be more descriptive about what it actually does. I also implemented what I thought was a very slight tweak to the logic for creating explicit children from latent ones.Previously, when we called
updateTileContenton a tile thatmightHaveLatentChildren(which is all tiles to start), we would give theTilesetContentLoaderassociated with the tile a chance to create explicit children. While looking at this in the above PR, I noted this was a bit dangerous in the case that the tile already has explicit children. It could end up clobbering the existing children, which would be very likely to lead to a hard to debug crash.Of course, none of our current loaders actually have this problem. Our current implicit loaders create one tile at a time, so every tile has no children to start, and so this situation never arises. Still,
TilesetContentLoaderis meant to be a generic interface, so I thought it was a good idea to defensively avoid this possibility.So I added a check: if the tile already has real children, don't call the loader to create more. It seemed like a good idea, and I thought it was safe.
Unfortunately this
mightHaveLatentChildrenflag has a side effect that I didn't consider, and my logic change meant that this flag stayedtruefor tiles with real children where previously it flipped tofalsethe first time the tiles were accessed. And as a result, some logic inupdateDoneStatethat turned placeholder raster overlays into real ones never executed. And that, in turn, would cause tilesets with raster overlays to never load at all in some cases, as in CesiumGS/cesium-unreal#1529.The fix in this PR simply makes
createLatentChildrenIfNecessarysetmightHaveLatentChildrento false when it sees a tile that already has real children. That allows the placeholder replacement logic to run, which unblocks loading.