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 styling + flight destination bug #8622

Merged
merged 2 commits into from
Feb 19, 2020
Merged

Conversation

lilleyse
Copy link
Contributor

Fixes #8618

Fixes two places where 3D Tiles code uses frameState.passes.render instead of the 3D Tiles-specific passOptions.render. Without this change the preload flight pass interferes with the style engine's dirty flag which is only supposed to be set to false in the render pass. The change in applyDebugSettings is not related to #8618 but was a similar type of bug.

@@ -1320,10 +1320,11 @@ import TileOrientedBoundingBox from './TileOrientedBoundingBox.js';
*
* @private
*/
Cesium3DTile.prototype.update = function(tileset, frameState) {
Cesium3DTile.prototype.update = function(tileset, frameState, passOptions) {
console.log(passOptions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was noticing absolutely terrible FPS and got scared that this was one of those "fix a bug, hurt performance" issues, but then I realized this stray console.log is destroying Sandcastle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry should have caught that. Fixed now.

@mramato
Copy link
Contributor

mramato commented Feb 19, 2020

I can confirm this fixes the issue and overall styling switches during flights feel much more "instant" now compared to before. I'll let @loshjawrence review and merge. Thanks!

@loshjawrence
Copy link
Contributor

loshjawrence commented Feb 19, 2020

These were all the places I saw framestate.passes.render in master (grepping from root)

Source/Scene/Batched3DModel3DTileContent.js|468| 45:        if ((commandStart < commandEnd) && (frameState.passes.render || frameState.passes.pick) && !defined(tileset.classificationType)) {
Source/Scene/SkyBox.js|108| 14:        if (!frameState.passes.render) {
Source/Scene/Instanced3DModel3DTileContent.js|465| 45:        if ((commandStart < commandEnd) && (frameState.passes.render || frameState.passes.pick)) {
Source/Scene/Scene.js|2866| 26:        var renderPass = frameState.passes.render;
Source/Scene/Scene.js|3264| 9:        frameState.passes.render = true;
Source/Scene/SkyAtmosphere.js|153| 14:        if (!frameState.passes.render) {
Source/Scene/Sun.js|124| 14:        if (!frameState.passes.render) {
Source/Scene/Globe.js|718| 13:        if (frameState.passes.render) {
Source/Scene/Globe.js|818| 13:        if (frameState.passes.render) {
Source/Scene/Cesium3DTile.js|1215| 14:        if (!frameState.passes.render) {
Source/Scene/Primitive.js|1820| 15:            (!frameState.passes.render && !frameState.passes.pick)) {
Source/Scene/Cesium3DTileStyleEngine.js|40| 13:        if (frameState.passes.render) {

Specs/createFrameState.js|28| 9:        frameState.passes.render = true;
Specs/Scene/ClassificationPrimitiveSpec.js|319| 9:        frameState.passes.render = false;
Specs/Scene/SkyAtmosphereSpec.js|152| 15:        scene.frameState.passes.render = false;
Specs/Scene/ImageryLayerCollectionSpec.js|268| 19:            scene.frameState.passes.render = true;
Specs/Scene/Cesium3DTilesetHeatmapSpec.js|30| 15:        scene.frameState.passes.render = true;
Specs/Scene/PointCloud3DTileContentSpec.js|55| 15:        scene.frameState.passes.render = true;
Specs/Scene/GroundPrimitiveSpec.js|331| 9:        frameState.passes.render = false;
Specs/Scene/SkyBoxSpec.js|101| 15:        scene.frameState.passes.render = false;
Specs/Scene/SunSpec.js|81| 15:        scene.frameState.passes.render = false;
Specs/Scene/PrimitiveSpec.js|75| 15:        scene.frameState.passes.render = true;
Specs/Scene/PrimitiveSpec.js|253| 9:        frameState.passes.render = false;
Specs/Scene/GroundPolylinePrimitiveSpec.js|295| 9:        frameState.passes.render = false;
Specs/Scene/Cesium3DTileSpec.js|341| 19:            scene.frameState.passes.render = true;

Just to confirm, we only care to use passOptions.render in tileset and tile code since they are aware of traversal passes and the other files don't care about traversal passes, correct?

@lilleyse
Copy link
Contributor Author

Just to confirm, we only care to use passOptions.render in tileset and tile code since they are aware of traversal passes and the other files don't care about traversal passes, correct?

Yeah that's right. I checked for all occurrences of passes.render as well and any other usages in 3D Tiles code is to help update / push commands which should work regardless of the 3D Tiles pass. There's a bigger cleanup effort needed for the concept of "passes" in Cesium, but that's for later.

@loshjawrence loshjawrence merged commit 47b0fb9 into master Feb 19, 2020
@loshjawrence loshjawrence deleted the style-with-flight-fix branch February 19, 2020 14:48
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.

Updating 3D Tiles style doesn't always update existing tiles
3 participants