-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Update New York CityGML 3D Tileset #6555
Conversation
shehzan10
commented
May 2, 2018
- Better streaming performance with draco compressed tiles.
- Uses new tiling algorithm with smarter tiling.
- Longitude/Latitude are in degrees so there is a conversion in styling.
- Add lon/lat/height to the feature picking table.
- Remove the area styling - no longer a property of the tileset.
* Better streaming performance with draco compressed tiles. * Uses new tiling algorithm with smarter tiling. * Longitude/Latitude are in degrees so there is a conversion in styling. * Add lon/lat/height to the feature picking table. * Remove the area styling - no longer a property of the tileset.
@shehzan10, thanks for the pull request! Maintainers, we have a signed CLA from @shehzan10, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
// Color buildings by their latitude coordinate. | ||
function colorByLatitude() { | ||
tileset.style = new Cesium.Cesium3DTileStyle({ | ||
defines: { | ||
latitudeRadians: "Number(${latitude}) * 0.0174532925" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use built in radians
function: https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/master/Styling#radians
["${height} >= 25", "rgb(252, 230, 200)"], | ||
["${height} >= 10", "rgb(248, 176, 87)"], | ||
["${height} >= 5", "rgb(198, 106, 11)"], | ||
["Number(${height}) >= 300", "rgba(45, 0, 75, 0.5)"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had talked about this earlier, but is the cast required? Does it suggest something is different about the new tileset?
@@ -105,14 +101,14 @@ | |||
// Color buildings with a '3' in their name. | |||
function colorByNameRegex() { | |||
tileset.style = new Cesium.Cesium3DTileStyle({ | |||
color : "(regExp('3').test(${name})) ? color('cyan', 0.9) : color('purple', 0.1)" | |||
color : "(regExp('3').test(String(${name}))) ? color('cyan', 0.9) : color('purple', 0.1)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the string cast required?
We should also wait to merge until Draco is supported in IE: #6404. |
Awesome. What's the status of Draco support in IE11? I assume this tileset won't load there? We should probably hold off on merging this until IE11 works. |
Marking this |
I'm generating a new tileset that uses PBR materials instead of WebGL extension. Please wait for my word before merging this. |
This is now ready for review and merge. |
Looks great @shehzan10, thanks! |