itowns icon indicating copy to clipboard operation
itowns copied to clipboard

Feat/google3d tiles

Open jogarnier opened this issue 1 year ago • 2 comments

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

https://github.com/iTowns/itowns/issues/2099

jogarnier avatar Jun 30 '23 08:06 jogarnier

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 #2183 is merged since I needed to load local files.

@Desplandis let me know what you think :)

jailln avatar Feb 01 '24 15:02 jailln

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 :grin: )
  • 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.

jailln avatar Feb 16 '24 10:02 jailln

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

Desplandis avatar Mar 15 '24 10:03 Desplandis