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

3D Tiles style fixes #6998

Merged
merged 1 commit into from
Sep 4, 2018
Merged

3D Tiles style fixes #6998

merged 1 commit into from
Sep 4, 2018

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Sep 3, 2018

This should be merged before the release tomorrow. It fixes bugs introduced after 1.48 so no CHANGES.md update needed.

The traversal cleanup PR #6390 had two small bugs with styling, the first one being a style always gets reapplied to a tile that was not selected the previous frame, and the second being a style always gets reapplied if the tile is selected during both the render and pick passes. These are both noticeable in the 3D Tiles Feature Picking demo.

The first bug happens when you click a building, zoom out so the tile is not selected, and zoom back in. The style gets reapplied incorrectly and the building is shown again.

bass2

The second bug happens when you click multiple buildings. The style gets applied every time a pick happens which causes all buildings to be shown.

bad

Sample code

@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

// Tile is newly selected; it is selected this frame, but was not selected last frame.
tile.lastStyleTime = 0; // Force applying the style to this tile
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was originally added so that expired tiles would get restyled but was responsible for the first bug. Now lastStyleTime is reset above in the // Refresh style for expired content area.

@@ -137,9 +137,8 @@ define([
tileContent.featurePropertiesDirty = false;
tile.lastStyleTime = 0; // Force applying the style to this tile
tileset._selectedTilesToStyle.push(tile);
} else if ((tile._selectedFrame !== frameState.frameNumber - 1)) {
} else if ((tile._selectedFrame < frameState.frameNumber - 1)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This caused the second bug. It wasn't accounting for the fact that the pick and render pass are the same frame number.

@ggetz
Copy link
Contributor

ggetz commented Sep 4, 2018

I can confirm this fixes both the issues. Thanks @lilleyse

@ggetz ggetz merged commit 764591a into master Sep 4, 2018
@ggetz ggetz deleted the style-apply-fix branch September 4, 2018 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants