Skip to content

Support Google Photorealistic 3D Tiles in iTwin.js #8104

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

Draft
wants to merge 65 commits into
base: master
Choose a base branch
from

Conversation

markschlosseratbentley
Copy link
Contributor

@markschlosseratbentley markschlosseratbentley commented May 15, 2025

Related to #8036.

iTwin.js can already attach tilesets from a variety of sources. Google Photorealistic 3d Tiles are in the common 3D Tiles format so in theory should work in iTwin.js. However, a few hurdles were found which this PR works around.

This PR adds a new RealityDataSourceG3DTImpl implementation of RealityDataSource which can be used to request and process Google Photorealistic 3D Tiles if you have a valid key to access them. This RealityDataSourceGP3DTImpl implementation does the following:

  • implements usesGeometricError as true which tells iTwin.js to use the geometric errors found inside the GP3DT tileset, which is crucial for good performance.
  • fixes some base URL handling for some situations found in GP3DT tilesets that RealityDataSourceTilesetUrlImpl did not handle properly.
  • fixes query param propagation for some situations found in GP3DT tilesets so the query params get passed down to children tiles from child json files. This could be fixed more thoroughly (to account for other possible arrangements of tile trees).
  • handles authentication for GP3DT tilesets using IModelApp.realityDataFormatRegistry.

Other changes:

  • Moves GoogleMapDecorator.ts from map-layers-formats to the core-frontend package, to use the decorator to display the logo in the bottom left of the viewport when the Google Photorealistic 3d Tiles are visible
  • Gets the optional copyright property when reading glTF files, which is passed into RealityModelTileTree and used to display attributions in accordance with Google's policy

Attributions screenshot:

image

To use this implementation, attach a reality model to your ViewState in the following manner:

// Before calling this code, you must configure IModelApp to have a valid GP3DT key in its
// instance of RealityDataFormatRegistry.
// See `IModelApp.realityDataFormatRegistry`.

// Also see `getGooglePhotorealistic3DTilesURL()` for a way to get the GP3DT URL you will
// need to specify to the call to `attachRealityModel()`.

const url = getGooglePhotorealistic3DTilesURL();
viewState.displayStyle.attachRealityModel({
  tilesetUrl: url,
  name: "googleMap3dTiles",
  rdSourceKey: {
      provider: "GP3DT",
      format: "ThreeDTile",
      id: url,
    },
  });

Alternatively, you can test this implementation using display-test-app after setting the IMJS_GP3DT_KEY environment variable to a proper value.

Enable the GP3DT tiles by toggling off Background Map and toggling on Google Photorealistic 3D Tiles in the view settings dialog:

image

TODO:

  • This PR, in the code, uses the acronym G3DT for Google Photorealistic 3d Tiles. Should we use GP3DT instead? I have seen both forms used. We have now decided to use GP3DT and this change has been pushed.
  • Should the base URL handling improvements also end up on RealityDataSourceTilesetUrlImpl? It should not. We should keep this separate for now, IMO. We need special handling for the GP3DT case including special auth handling that the TilesetUrl implementation does not handle.
  • Properly handle attribution data for the GP3DT tiles that are currently visible.
  • Add Google logo in view itself, associated with the attribution data for GP3DT tiles.
  • Consider proper code location of Google logo decorator.
  • Consider how the glTF tracking code is done --- copyright is currently in the Tile class - is this okay?
  • Clean up the glTF copyright tracking code -- this is not guarded behind GP3DT, and that is probably fine. Remove the dead code which tries to guard it behind GP3DT.
  • Instead of adding gp3dtKey onto TileAdmin, look into implementing something similar to MapLayerFormatRegistry.ts.
  • ImageTests
  • Add unit tests

TODO (later PRs):

  • How can we do the query param propagation in a way that actually considers the entire tree structure, rather than just checking for json files? (see code for how it works, and comments). We probably should consider this for a future PR to consolidate things further. Not necessary for getting this up and running quickly in iTwin.js.
  • Add a flag which displays attribution data for GP3DT tiles in a line at the bottom left of the viewport displaying them.

@matmarchand
Copy link
Contributor

Thanks for your effort on this.
I understand the careful approach of having a separate source implementation but I feel like RealityDataSourceTilesetUrlImpl and RealityDataSourceG3DTImpl should merge since they are based on the same standard. In that sense, maybe we should avoid exposing new APIs and hide that complexity internally by enabling Google behavior based on the url?

Unrelated to this PR but I'm surprised that GeometricError is not enabled by default. Would we get a performance improvement for reality mesh as well?

@pmconne
Copy link
Member

pmconne commented May 15, 2025

Unrelated to this PR but I'm surprised that GeometricError is not enabled by default. Would we get a performance improvement for reality mesh as well?

Yes, at the expense of drawing much lower-resolution tiles for the vast majority of reality models used with iTwins.
Apparently most reality models produced by Bentley have been created with a target error of 1 versus Cesium's 16. So they render with appropriate LOD in iTwin.js and need to override the default error to render properly in CesiumJS.
I think some changes were made in that area recently - can you check with that team?

@markschlosseratbentley
Copy link
Contributor Author

markschlosseratbentley commented May 16, 2025

Thanks for your effort on this. I understand the careful approach of having a separate source implementation but I feel like RealityDataSourceTilesetUrlImpl and RealityDataSourceG3DTImpl should merge since they are based on the same standard. In that sense, maybe we should avoid exposing new APIs and hide that complexity internally by enabling Google behavior based on the url?

Doing everything in RealityDataSourceTilesetUrlImpl was the original approach, but when we found we needed to define different behavior for usesGeometricError for Google Photorealistic 3D Tiles, we made a separate source implementation.

We could just put all of this code in RealityDataSourceTilesetUrlImpl and conditionally trigger it based on the URL, but what if the GP3DT URL changes in the future?

I also hesitate to throw all of this into RealityDataSourceTilesetUrlImpl because this comment indicates that RealityDataProvider.TilesetUrl is considered legacy.

I generally agree that the proper handling of all of this stuff should eventually also go into other source implementations that parse 3d Tiles format tiles, probably getting combined into one implementation.

Should that work be in a followup PR? (This current fix is not 100% proper, it just gets GP3DT up and running). Edit: we could tag this implementation alpha for easy followup changes.

Edit: there is also a concern that altering the core logic of RealityDataSourceTilesetUrlImpl could break other cases (but we can extensively test to be sure we're okay). But a separate implementation feels safer to put out there quickly.

@pmconne
Copy link
Member

pmconne commented May 19, 2025

const url = `https://tile.googleapis.com/v1/3dtiles/root.json?key=YOUR_GOOGLE_3D_TILES_KEY_GOES_HERE`;
viewState.displayStyle.attachRealityModel({
  tilesetUrl: url,

The url gets stored in the display style JSON. People will not want to store their Google 3D Tiles API key in there. The primary job of the various map and reality data "provider" objects is to handle authentication and format URLs. Shouldn't your provider implement those similarly to RealityDataSourceCesiumIonAssetImpl?

@markschlosseratbentley
Copy link
Contributor Author

const url = `https://tile.googleapis.com/v1/3dtiles/root.json?key=YOUR_GOOGLE_3D_TILES_KEY_GOES_HERE`;
viewState.displayStyle.attachRealityModel({
  tilesetUrl: url,

The url gets stored in the display style JSON. People will not want to store their Google 3D Tiles API key in there. The primary job of the various map and reality data "provider" objects is to handle authentication and format URLs. Shouldn't your provider implement those similarly to RealityDataSourceCesiumIonAssetImpl?

Agree.

@eringram
Copy link
Member

@markschlosseratbentley I added attributions ordered by number of occurrences. If there are copyright properties in the tiles but the tree provider is not RealityDataProvider.G3DT, it still displays them but without the Google header and icon. If there are no copyrights there will be no new logo card added to this list.

image

I also started working on adding the logo as a decorator on top of the view itself. GoogleMapsImageryProvider uses GoogleMapsDecorator to achieve this which I think we should reuse, but it's in the map-layer-formats package which we don't want to import into core-frontend, so maybe that class can be moved (it is internal). I'll try that tomorrow morning

* A registry of RealityDataFormats identified by their unique format IDs. The registry can be accessed via [[IModelApp.realityDataFormatRegistry]].
* @alpha
*/
export class RealityDataFormatRegistry {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be a provider registry not format - google's tiles are in the same format as ION's, Context Capture's, etc - what differs is the provider logic (authentication, url formatting, etc). That would match with RealityDataSourceKey.provider.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't appear to expose any way for apps / other packages to register their own providers. All it seems to do is provide a place to register and later look up a google API key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My takeaways from this feedback:

  1. Make this be RealityDataProviderRegistry.
  2. Add something like this to RealityDataOptions.

Is there something further you are thinking of? It does just basically wrap an auth key right now.

Copy link
Member

@pmconne pmconne May 22, 2025

Choose a reason for hiding this comment

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

No, the crucial thing your registry is missing compared to MapLayerFormatRegistry is the register function. The "config" thing with key-value pairs IMO is not great in either registry; it'd make more sense to me for somebody to instantiate a Google3DTilesProvider, passing it the API key and whatever else it needs, and then register that provider with your registry. When we encounter a RealityDataSourceKey we'd look up the provider in the registry by its name and go from there. This implies that we'd also register an ION provider etc.

Copy link
Member

@eringram eringram May 23, 2025

Choose a reason for hiding this comment

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

@pmconne

Given that we already have RealityDataSourceProvider, is this roughly what you are envisioning? (overview, this code is not meant to include everything it would need)

export class RealityDataSourceGP3DTProvider implements RealityDataSourceProvider {
  private _apiKey: string;

  // api key would be passed into RealityDataSourceGP3DTImpl or something
  // to use it in query params, auth header, etc.

  public async createRealityDataSource(key: RealityDataSourceKey, iTwinId: GuidString | undefined): Promise<RealityDataSource | undefined> {
    return RealityDataSourceGP3DTImpl.createFromKey(key, iTwinId);
  }

  public constructor(apiKey: string) {
    this._apiKey = apiKey;
  }
}

// Usage:
IModelApp.realityDataSourceProviders.register("GP3DT", new RealityDataSourceGP3DTProvider("my api key"));

If so, is clarity for how to pass in the API key the main purpose? We are already automatically registering the source like this, but the approach in this snippet would force the user to set their API key in the provider instead of elsewhere.

}

// @alpha
export interface RealityDataKey {
Copy link
Contributor

@matmarchand matmarchand May 22, 2025

Choose a reason for hiding this comment

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

For our use case, authorization will be provided via authorization header instead of api key. Could we add support for authorization header as well?

Copy link
Member

Choose a reason for hiding this comment

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

What kind of authorization - like a bearer token, and from what service (Bentley IMS/other)? And can this replace the API key? From what I understand, the API key is required for all the Google 3D Tiles requests, so I'm not sure how they would be accessed without the user providing one.

Maybe this is also relevant to @aruniverse's comment below. I don't think the Google Photorealistic 3D Tiles allow you to use a session token in the same way as the 2D Map Tiles. I don't see a mention of sessions in the 3D Tiles docs, and see this in the 2D Tiles:

Note: You need to use session tokens for getting 2D Tiles and Street View Tiles, but not for getting 3D Tiles.

Let me know if we're missing a puzzle piece here though, and there's another service that can provide auth/sessions in this context

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes a Bearer token in the authorization header. ref: https://developers.google.com/maps/documentation/tile/oauth_token

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying. In this commit I added an option so when the provider is created, the user can pass in a function that returns an authorization token. That function is called to provide the token for the authorization header when the google tiles are requested.

Working on cleaning this up to make the API key optional (I assume it won't be required in the auth header case?) and add documentation

@aruniverse aruniverse added this to the iTwin.js 5.1 milestone May 27, 2025
@eringram eringram requested a review from a team as a code owner May 28, 2025 19:20
Copy link
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

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

not sure if this pr is actually ready for review, and still needs to be cleaned up to not cut corners like paul mentioned elsewhere. adding a few minor comments.

if not ready, please draft

Copy link
Member

Choose a reason for hiding this comment

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

you would need to copy the assets this decorator needs from map-layers-fromats into core-frontend

Copy link
Member

Choose a reason for hiding this comment

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

Fixed, thanks, good catch. I guess this didn't show up as an error DTA because the assets were also included from map-layers-formats?

Copy link
Member

Choose a reason for hiding this comment

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

revert whitespace polluting diff

Copy link
Member

Choose a reason for hiding this comment

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

Done

*/
export interface RealityDataProviderOptions {
/** Access key for Google Photorealistic 3D Tiles in the format `{ key: "key", value: "your-gp3dt-key" }`. */
gp3dt?: RealityDataProviderKey;
Copy link
Member

Choose a reason for hiding this comment

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

should we be hardcoding these in core-frontend?
also take into account those who will not be providing an access token/key, but has to interface with a service to obtain a session token. you should look at what was done for google 2d tiles

Copy link
Contributor Author

@markschlosseratbentley markschlosseratbentley May 29, 2025

Choose a reason for hiding this comment

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

Based on earlier feedback (and now your feedback as well), we are in the process of changing the way this works. New approach:

  1. User can create a GP3DT provider (new API class)
  2. User can register that provider themselves if they want to use it. (The RealityDataProviderOptions / related structures will go away).
  3. User will be responsible for passing in a valid key, if desired.
  4. An alternate using a session token / authorization header will be available - but we need to develop this.

Stay tuned for further code updates in these areas (the first three of these are done; we need to clean up old classes still).

@@ -221,6 +229,11 @@ export namespace RealityDataSource {
* is invoked to produce the reality data source.
* @alpha
*/

// TODO extend this for GP3DT
Copy link
Member

Choose a reason for hiding this comment

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

how applicable are these todos still?

Copy link
Member

Choose a reason for hiding this comment

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

RealityDataSourceGP3DTProvider is still technically WIP so I reverted this to draft

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reverting to draft, @eringram. Thanks @aruniverse for the feedback - we were unsure how fast we would try to push this through until we had some more discussions.

// Only add another logo card if the tiles have copyright
if (copyrightMap.size > 0) {
// Order by most occurances to least
// See https://developers.google.com/maps/documentation/tile/create-renderer#display-attributions
Copy link
Member

Choose a reason for hiding this comment

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

based off of this, i wonder if we need an additional / clickable data attributions thats always visible on the vp and when clicked also shows the logo card

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference; more details about attribution requirements are discussed here: #8036 (comment)

@eringram eringram marked this pull request as draft May 28, 2025 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants