Feat/google3d tiles #2124

jogarnier · 2023-06-30T08:58:55Z

Description

added C3DTilesGoogleSource type to C3DTilesSource.
C3DTilesGoogleSource requires non-standard parsing metadata to provides valid urls => identified this code clearly
Refactored b3dmParser to extract pure gltfparser logic
added url parameters to Fetcher methods to allow passing dynamic parameters to url request, (required by C3DTilesGoogleSource authentification logic. https://developers.google.com/maps/documentation/tile/create-renderer?hl=en)

Motivation and Context

#2099

docs/tutorials/Fundamentals.md

src/Parser/GLTFParser.js

src/Process/3dTilesProcessing.js

src/Provider/3dTilesProvider.js

src/Source/C3DTilesGoogleSource.js

src/Parser/GLTFParser.js

Desplandis

Nice job on this PR! =)
I was able to test it using my google cloud keys and your example branch. This is still "experimental" as they are critical issues (blinking, no loading of some tiles, ...) but these come from our 3D tiles implementation.

As for my comments:

I think you should move some parts specific to b3dm outside of the gltf parser
I question if we should follow the standard or cesium behavior
I'm not convinced by the multiple changes in Fetcher.js due to this use-case being specific but I could change my mind with a little explanation =)
Some minor issues

P.S.: Before merging, could you please rewrite your commit messages + your history (notably isolate changes in Fetcher.js, changes in b3dm/gltf parsers in their own commits)?

src/Parser/B3dmParser.js

src/Parser/GLTFParser.js

src/Process/3dTilesProcessing.js

src/Provider/Fetcher.js

src/Source/C3DTilesGoogleSource.js

jailln · 2024-02-01T15:32:20Z

I just pushed some modifications that should answer all of your comments:

GLTFParser is now only a wrapper for gltfLoader and legacyGltfLoader and everything 3D tiles related as been moved to B3DMParser
Only export GLTFParser and not gltfLoader and legacyGltfLoader anymore, which is a breaking change but avoids exporting two things having the same purpose. WDYT ? Should we keep it like that or also export gltfLoader and legacyGltfLoader to avoid a breaking change until the next major version?
Some 3D Tiles related parts could be refactored / simplified (especially the B3dmParser) but I didn't go into much trouble here since we are seriously considering to handle 3D tiles parsing with an external lib very soon.
I wanted to add more tests on the GLTFParser but I decided to wait until rework unit tests (mocking the fetch()) #2183 is merged since I needed to load local files.

@Desplandis let me know what you think :)

Desplandis

Nice changes, I did have some additional comments nonetheless but should be easy to answer. Since those are mostly aesthetics + some questions, I'll approve this PR but please answer them. =)

Only export GLTFParser and not gltfLoader and legacyGltfLoader anymore, which is a breaking change but avoids exporting two things having the same purpose. WDYT ? Should we keep it like that or also export gltfLoader and legacyGltfLoader to avoid a breaking change until the next major version?

I'm usually cautious about breaking changes, however I don't think they are used directly by anyone...

Some 3D Tiles related parts could be refactored / simplified (especially the B3dmParser) but I didn't go into much trouble here since we are seriously considering to handle 3D tiles parsing with an external lib very soon.

I held comments on that topic. x)

I wanted to add more tests on the GLTFParser but I decided to wait until #2183 is merged since I needed to load local files.

This will the object of a future PR I think!

Desplandis · 2024-02-07T17:03:25Z

src/Parser/B3dmParser.js

     * true, a threejs [MeshBasicMaterial](https://threejs.org/docs/index.html?q=meshbasic#api/en/materials/MeshBasicMaterial)
     * is set up. config.overrideMaterials can also be a threejs [Material](https://threejs.org/docs/index.html?q=material#api/en/materials/Material)
     * in which case it will be the material used to override.
     * @return {Promise} - a promise that resolves with an object containig a THREE.Scene (gltf) and a batch table (batchTable).
     *
     */
    parse(buffer, options) {
-        const gltfUpAxis = options.gltfUpAxis;
-        const urlBase = options.urlBase;
+        const frustumCulled = options.frustumCulled === undefined || options.frustumCulled === null ? true : !!(options.frustumCulled);


A simpler way could be:

Suggested change

const frustumCulled = options.frustumCulled === undefined || options.frustumCulled === null ? true : !!(options.frustumCulled);

const frustumCulled = options.frustumCulled ?? true;

Moreover, the default parameter is documented as false.

Set it back to false and used a more syntetic syntax

Desplandis · 2024-02-07T17:11:43Z

src/Provider/3dTilesProvider.js

        };
        return Fetcher.arrayBuffer(url, layer.source.networkOptions).then((result) => {
            if (result !== undefined) {
                let func;
                const magic = utf8Decoder.decode(new Uint8Array(result, 0, 4));
                if (magic[0] === '{') {
                    result = JSON.parse(utf8Decoder.decode(new Uint8Array(result)));
-                    const newPrefix = url.slice(0, url.lastIndexOf('/') + 1);
+                    let newPrefix = '';


No need to assign newPrefix since it is assigned in both branch of the following condition (or do a ternary condition).

Suggested change

let newPrefix = '';

let newPrefix;

Desplandis · 2024-02-07T17:20:14Z

src/Source/C3DTilesGoogleSource.js

+        const uri = tile.content.uri.substr(tile.content.uri.indexOf('?') + 1);
+        const sessionIds = uri.split('=');
+        return sessionIds[1];


Is there a non-hacky way to do this with URLSearchParams and get this value with the corresponding key (i.e. sessionId)?

src/Source/C3DTilesGoogleSource.js

src/Parser/B3dmParser.js

Desplandis · 2024-02-07T17:26:57Z

src/Main.js

@@ -91,7 +92,7 @@ export { default as LASParser } from 'Parser/LASParser';
 export { default as ISGParser } from 'Parser/ISGParser';
 export { default as GDFParser } from 'Parser/GDFParser';
 export { default as GTXParser } from 'Parser/GTXParser';
-export { enableDracoLoader, enableKtx2Loader, glTFLoader, legacyGLTFLoader } from 'Parser/B3dmParser';
+export { default as GLTFParser, enableDracoLoader, enableKtx2Loader } from 'Parser/GLTFParser';


Okay with the change!

jailln · 2024-02-16T10:28:41Z

I went back on a previous change: I keep exporting gltfLoader and legacyGLTFLoader for now, for several reasons:

GLTFParser only has a parse method and not the other methods of gltfLoader and legacyGLTFLoader (e.g. load, loadAsync, etc.) that itowns users might use (including me 😁 )
don't introduce an unnecessary breaking change now
this is a bit out of scope of this PR

Therefore I think it's ready to merge @Desplandis

Note that I started a branch to migrate to 3d-tiles-renderer-js in which I started implementing an "itowns GltfLoader" that inherits from THREE.Loader, shares the same interface than GLTFLoader but can load gltf 1.0 and 2.0 files. It makes more sense to do it in the 3d-tiles-renderer-js migration since we really need it in this context.

Desplandis

Sounds good to me, all comments have been resolved! =)

Desplandis · 2024-03-15T10:46:26Z

@jailln Since #2183 was merged before this PR, you have a conflict in the GLTFLoader unit test but it should be easy to resolve!

jogarnier requested review from gchoqueux and jailln June 30, 2023 08:58

jogarnier force-pushed the feat/google3dTiles branch from 183f629 to fd7c586 Compare June 30, 2023 09:02

jailln requested a review from mgermerie June 30, 2023 09:03

jogarnier force-pushed the feat/google3dTiles branch from fd7c586 to 960d517 Compare June 30, 2023 09:03

jailln reviewed Jul 3, 2023

View reviewed changes

jogarnier force-pushed the feat/google3dTiles branch 5 times, most recently from 3dc77f9 to 9d4c59b Compare July 3, 2023 15:35

jailln reviewed Jul 13, 2023

View reviewed changes

src/Parser/GLTFParser.js Outdated Show resolved Hide resolved

jogarnier force-pushed the feat/google3dTiles branch from 9d4c59b to 49a5362 Compare July 18, 2023 16:00

jailln mentioned this pull request Aug 23, 2023

feat(3dtiles): add method to enable ktx2 loader for gltf #2172

Merged

jailln assigned Desplandis Sep 19, 2023

jogarnier mentioned this pull request Oct 2, 2023

Feat/google3d tile example #2198

Closed

Desplandis force-pushed the feat/google3dTiles branch 2 times, most recently from 5ee5ca7 to 5103ada Compare November 20, 2023 10:43

Desplandis requested changes Nov 20, 2023

View reviewed changes

jailln self-assigned this Jan 22, 2024

jailln force-pushed the feat/google3dTiles branch 2 times, most recently from 4028887 to f2d6a18 Compare February 1, 2024 15:23

jailln requested a review from Desplandis February 1, 2024 15:32

jailln force-pushed the feat/google3dTiles branch from f2d6a18 to 9b8011e Compare February 1, 2024 15:41

Desplandis reviewed Feb 7, 2024

View reviewed changes

jailln force-pushed the feat/google3dTiles branch from 9b8011e to c91731d Compare February 12, 2024 17:08

jailln requested a review from Desplandis February 12, 2024 17:08

jailln force-pushed the feat/google3dTiles branch 2 times, most recently from 0d9430d to 7f1146d Compare February 12, 2024 17:49

jailln force-pushed the feat/google3dTiles branch 2 times, most recently from 319443e to a6f1b6e Compare February 16, 2024 10:22

jailln force-pushed the feat/google3dTiles branch from a6f1b6e to bacf9cc Compare February 16, 2024 10:34

jailln force-pushed the feat/google3dTiles branch 3 times, most recently from 3eee948 to d5ae6cb Compare March 6, 2024 13:46

Desplandis approved these changes Mar 15, 2024

View reviewed changes

jailln force-pushed the feat/google3dTiles branch from d5ae6cb to 974e229 Compare March 15, 2024 11:44

jailln removed request for gchoqueux and mgermerie March 15, 2024 11:45

jailln force-pushed the feat/google3dTiles branch from 974e229 to ceaa063 Compare March 15, 2024 16:25

feat(gltf): add a GLTFParser to parse gltf 1.0 and 2.0 files

f5c3168

jailln force-pushed the feat/google3dTiles branch from ceaa063 to f5c3168 Compare March 15, 2024 16:44

jailln merged commit e6eb4cf into iTowns:master Mar 15, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/google3d tiles #2124

Feat/google3d tiles #2124

jogarnier commented Jun 30, 2023

Desplandis left a comment •

edited

Loading

jailln commented Feb 1, 2024

Desplandis left a comment •

edited

Loading

Desplandis Feb 7, 2024

jailln Feb 12, 2024

Desplandis Feb 7, 2024

Desplandis Feb 7, 2024

jailln Feb 12, 2024

Desplandis Feb 7, 2024

jailln commented Feb 16, 2024 •

edited

Loading

Desplandis left a comment

Desplandis commented Mar 15, 2024

	const frustumCulled = options.frustumCulled === undefined \|\| options.frustumCulled === null ? true : !!(options.frustumCulled);
	const frustumCulled = options.frustumCulled ?? true;

Feat/google3d tiles #2124

Feat/google3d tiles #2124

Conversation

jogarnier commented Jun 30, 2023

Description

Motivation and Context

Desplandis left a comment • edited Loading

Choose a reason for hiding this comment

jailln commented Feb 1, 2024

Desplandis left a comment • edited Loading

Choose a reason for hiding this comment

Desplandis Feb 7, 2024

Choose a reason for hiding this comment

jailln Feb 12, 2024

Choose a reason for hiding this comment

Desplandis Feb 7, 2024

Choose a reason for hiding this comment

Desplandis Feb 7, 2024

Choose a reason for hiding this comment

jailln Feb 12, 2024

Choose a reason for hiding this comment

Desplandis Feb 7, 2024

Choose a reason for hiding this comment

jailln commented Feb 16, 2024 • edited Loading

Desplandis left a comment

Choose a reason for hiding this comment

Desplandis commented Mar 15, 2024

Desplandis left a comment •

edited

Loading

Desplandis left a comment •

edited

Loading

jailln commented Feb 16, 2024 •

edited

Loading