Update sample tilesets to version 1.0 #6857

lilleyse · 2018-07-30T14:31:51Z

Updates all of the sample tilesets in Specs/Data/Cesium3DTiles to 3D Tiles 1.0 and glTF 2.0. Includes some bug fixes as well. @ggetz can you review?

CC CesiumGS/3d-tiles-validator#139

cesium-concierge · 2018-07-30T14:31:52Z

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.
❔ Changes to third party files were made.
- Looks like a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version.

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.

🌍 🌎 🌏

lilleyse · 2018-07-30T14:35:43Z

Source/Scene/Cesium3DTileBatchTable.js

            // E.g. texture2D(u_diffuse, uv)
            // E.g. texture2D(u_diffuse, computeUV(index))
-            regex = new RegExp('texture2D\\(' + diffuseAttributeOrUniformName + '.*?(\\)\\)|\\))', 'g');
-            source = source.replace(regex, 'tile_diffuse_final($&, tile_diffuse)');
+            source = replaceDiffuseTextureCalls(source, diffuseAttributeOrUniformName);


The regex was too fragile before. It failed for a GLSL line from processPbrMetallicRoughness. Updated to support any number of nested parentheses.

I didn't see a spec update for this. Can we add a test with the offending line since it sounds like a good test case?

This code is tested in Cesium3DTilesetSpec with the colorBlendMode tests. I checked that this area of the code has 100% code coverage (the file is 99%).

sets colorBlendMode for textured tileset passes in master, fails without the replaceDiffuseTextureCalls fix, and passes with the replaceDiffuseTextureCalls fix. So no update is required.

lilleyse · 2018-07-30T14:36:49Z

Source/Scene/ClassificationModel.js

+            gltf = parseBinaryGltf(gltf); // Updates to 2.0 and adds pipeline extras
+            addDefaults(gltf);
+            processModelMaterialsCommon(gltf);
+            processPbrMetallicRoughness(gltf);


Added these lines because other code in ClassificationModel expected gltf.techniques to exist.

@ggetz you may need to add these changes to #6805 as well.

Noted in that PR.

lilleyse · 2018-07-30T14:38:19Z

Source/ThirdParty/GltfPipeline/addDefaults.js

+        if (gltf.asset.extras.gltf_pipeline_upgrade_10to20) {
+            addDefaultMaterial(gltf);
+            addDefaultTechnique(gltf);
+        }


This is sort of a hack - this tells gltf-pipeline to only add default materials or techniques if the model was a 1.0 model. If the model has pbrMetallicRoughness these lines are not called. This was added to fix a test where 2 shaders were being created instead of 1.

This is temporary until #6805.

lilleyse · 2018-07-30T14:38:39Z

Source/ThirdParty/GltfPipeline/processPbrMetallicRoughness.js

+        if (optimizeForCesium) {
+            var baseColorParameter = defaultValue(techniqueParameters.baseColorTexture, techniqueParameters.baseColorFactor);
+            if (defined(baseColorParameter)) {
+                baseColorParameter.semantic = '_3DTILESDIFFUSE';


This is required for tileset.colorBlendMode to work for 2.0 models.

lilleyse · 2018-07-30T14:39:43Z

Specs/Scene/Batched3DModel3DTileContentClassificationSpec.js

@@ -146,16 +145,4 @@ defineSuite([
        });
    });

-    it('renders with quantization', function() {


Removed - no longer creating samples with glTF 1.0 extensions.

lilleyse · 2018-07-30T14:40:39Z

Specs/Scene/Batched3DModel3DTileContentSpec.js

    it('renders with textures', function() {
        return Cesium3DTilesTester.loadTileset(scene, texturedUrl).then(function(tileset) {
            Cesium3DTilesTester.expectRender(scene, tileset);
        });
    });

-    it('renders with compressed textures', function() {


Removed - no longer testing compressed textures in 3D Tiles 1.0 until a better glTF extension comes around.

lilleyse · 2018-07-30T14:40:58Z

Specs/Scene/Batched3DModel3DTileContentSpec.js

-        });
-    });
-
-    it('renders with a gltf z-up axis', function() {


Removed - no longer supporting gltfUpAxis in 3D Tiles 1.0.

lilleyse · 2018-07-30T14:42:50Z

Specs/Scene/Batched3DModel3DTileContentSpec.js

-            var geometryByteLength = 8880;
+            // 10 buildings, 36 ushort indices and 24 vertices per building, 8 float components (position, normal, uv) and 1 uint component (batchId) per vertex.
+            // 10 * ((24 * (8 * 4 + 1 * 4)) + (36 * 2)) = 9360
+            var geometryByteLength = 9360;


Due to glTF 2.0 restrictions on accessor layout, the _BATCHID accessor is now uint.

Other things that changed: textures are now 128x128 and uvs are removed from tilesets that aren't textured. Throughout there are changes to memory usage tests to account for this.

lilleyse · 2018-07-30T14:43:18Z

Specs/Scene/Cesium3DTilesetSpec.js

@@ -119,7 +119,7 @@ defineSuite([
    var tilesetReplacementWithViewerRequestVolumeUrl = 'Data/Cesium3DTiles/Tilesets/TilesetReplacementWithViewerRequestVolume/tileset.json';

    var tilesetWithExternalResourcesUrl = 'Data/Cesium3DTiles/Tilesets/TilesetWithExternalResources/tileset.json';
-    var tilesetUrlWithContentUri = 'Data/Cesium3DTiles/Tilesets/TilesetWithContentUri/tileset.json';
+    var tilesetUrlWithContentUri = 'Data/Cesium3DTiles/Batched/BatchedWithContentDataUri/tileset.json';


3d-tiles-samples-generator was outputting as Batched/BatchedWithContentDataUri/tileset.json so I switched the url.

lilleyse · 2018-07-30T14:44:50Z

Specs/Scene/Cesium3DTilesetSpec.js

-                expect(rgba[2]).toEqual(0);
+                expect(rgba[0]).toBeGreaterThan(200);
+                expect(rgba[1]).toBeLessThan(25);
+                expect(rgba[2]).toBeLessThan(25);


The colors are updated since pbr shading doesn't produce the same sort of solid shading as before.

ggetz

Cesium3DTileContent (and spec) are still checking content.url instead of content.uri. That should be updated correct? (I missed that in my first PR).

Otherwise looks good. Tests pass with for both source and release, and the updated tilesets all work in sandcastle and don't log errors to the console (unless they specifically have deprecated content).

ggetz · 2018-07-30T15:45:42Z

Source/Scene/Cesium3DTileBatchTable.js

            // E.g. texture2D(u_diffuse, uv)
            // E.g. texture2D(u_diffuse, computeUV(index))
-            regex = new RegExp('texture2D\\(' + diffuseAttributeOrUniformName + '.*?(\\)\\)|\\))', 'g');
-            source = source.replace(regex, 'tile_diffuse_final($&, tile_diffuse)');
+            source = replaceDiffuseTextureCalls(source, diffuseAttributeOrUniformName);


I didn't see a spec update for this. Can we add a test with the offending line since it sounds like a good test case?

lilleyse · 2018-07-30T17:01:03Z

Cesium3DTileContent (and spec) are still checking content.url instead of content.uri. That should be updated correct? (I missed that in my first PR).

At least for this PR, let's keep it content.url. Maybe a future PR could address this with the usual deprecation period.

ggetz · 2018-07-30T19:46:59Z

Ok, looks good to go then. Thanks @lilleyse !

lilleyse requested a review from ggetz July 30, 2018 14:31

lilleyse force-pushed the 1.0-tilesets branch from fa7b7ff to a4d7cba Compare July 30, 2018 14:33

Updated sample tilesets to 1.0

7e12268

lilleyse force-pushed the 1.0-tilesets branch from a4d7cba to 7e12268 Compare July 30, 2018 14:34

lilleyse commented Jul 30, 2018

View reviewed changes

lilleyse mentioned this pull request Jul 30, 2018

glTF 2.0 in samples generator CesiumGS/3d-tiles-validator#139

Merged

4 tasks

ggetz mentioned this pull request Jul 30, 2018

Update gltf Pipeline and support KHR_techniques_webgl and KHR_blend #6805

Merged

4 tasks

Fix test

2d14bfb

ggetz requested changes Jul 30, 2018

View reviewed changes

ggetz approved these changes Jul 30, 2018

View reviewed changes

ggetz merged commit b83c2de into master Jul 30, 2018

ggetz deleted the 1.0-tilesets branch July 30, 2018 19:47

lilleyse mentioned this pull request Aug 10, 2018

Fix model crash by adding default pbr material #6906

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update sample tilesets to version 1.0 #6857

Update sample tilesets to version 1.0 #6857

lilleyse commented Jul 30, 2018

cesium-concierge commented Jul 30, 2018 •

edited by ggetz

Loading

lilleyse Jul 30, 2018

ggetz Jul 30, 2018

lilleyse Jul 30, 2018

lilleyse Jul 30, 2018

lilleyse Jul 30, 2018

ggetz Jul 30, 2018

lilleyse Jul 30, 2018

lilleyse Jul 30, 2018

lilleyse Jul 30, 2018

lilleyse Jul 30, 2018

lilleyse Jul 30, 2018

lilleyse Jul 30, 2018

lilleyse Jul 30, 2018

lilleyse Jul 30, 2018

ggetz left a comment

ggetz Jul 30, 2018

lilleyse commented Jul 30, 2018

ggetz commented Jul 30, 2018

Update sample tilesets to version 1.0 #6857

Update sample tilesets to version 1.0 #6857

Conversation

lilleyse commented Jul 30, 2018

cesium-concierge commented Jul 30, 2018 • edited by ggetz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Jul 30, 2018

ggetz commented Jul 30, 2018

cesium-concierge commented Jul 30, 2018 •

edited by ggetz

Loading