itwinjs-core icon indicating copy to clipboard operation
itwinjs-core copied to clipboard

Add support for Google Photorealistic 3d Tiles

Open aruniverse opened this issue 10 months ago • 24 comments

As of https://github.com/iTwin/itwinjs-core/pull/7604 we support Google Maps 2D Tiles. We should look to add support for Google Photorealistic 3D Tiles.

Areas of concern we need to investigate is how to work with terrain and draped imagery / reality data / etc

aruniverse avatar May 02 '25 17:05 aruniverse

@aruniverse can we break this down into a series of task to review the areas of concern and how we might address them. Given that we will want to add support for GP3DT as we move towards providing a consistent view / geospatial context across our applications.

dkbraig avatar May 07 '25 15:05 dkbraig

I wanted to see if we support GP3DT dataset in iTwin.js so similar to what can be done in Cesium: Cesium.Cesium3DTileset.fromUrl("https://tile.googleapis.com/v1/3dtiles/root.json?key=XXX");

I attached GP3DT in iTwin.js: viewport.displayStyle.attachRealityModel( { tilesetUrl: "https://tile.googleapis.com/v1/3dtiles/root.json?key=XXX", name: "googleMap3dTiles" });

The root will load fine but it will fail to load sub-tiles. Example: 'https://tile.googleapis.com/v1/3dtiles//v1/3dtiles/datasets/CgA/files/UlRPVEYuYnVsa21ldGFkYXRhLnBsYW5ldG9pZD1lYXJ0aCxidWxrX21ldGFkYXRhX2Vwb2NoPTk5NyxwYXRoPSxjYWNoZV92ZXJzaW9uPTYsYWxpZ25tZW50X3ZlcnNpb249Uk9DS1RSRUVfOTk3X0dPT0dMRV9EQVRVTV8yMDI1MDIwNlQxNDUzWl9nZW5lcmF0ZWRfYXRfMjAyNTA0MThUMTIwOFo.json?session=CJ2fmueU_ILr_wEQpIjvwAY

Problems:

  1. The API key was not propagated.
  2. Sub tile URL was not properly constructed. ...googleapis.com/v1/3dtiles//v1/3dtiles/datasets/CgA/files...

So on top of handling the Google curated API authentication, terrain and drape imagery... we also need to look into supporting this specific dataset in our Cesium3dTiles implementation.

cc: @calebmshafer

matmarchand avatar May 07 '25 16:05 matmarchand

Problems:

1. The API key was not propagated.

Seems like similar problem that @mdastous-bentley solved for Cesium ION URLs.

2. Sub tile URL was not properly constructed. ...googleapis.com/v1/3dtiles//v1/3dtiles/datasets/CgA/files...

Seems like a URL parsing bug, can we see an example of a child tile URL supplied by the root tileset?

pmconne avatar May 12 '25 17:05 pmconne

I just spoke with @matmarchand on a call.

See: RealityDataSourceTilesetUrlImpl.ts Major problems:

  1. Session IDs are not propagated from any .json beyond the root level .json. In the case of Google 3D tiles, this needs to happen; we encounter sub-jsons at lower levels with meaningful params that need to get picked up.
  2. Same with API keys propagating.
  3. The relative URIs are not correct for sub-levels.

@matmarchand please feel free to add anything I missed.

markschlosseratbentley avatar May 12 '25 18:05 markschlosseratbentley

Pushed my investigation branch https://github.com/iTwin/itwinjs-core/tree/MathieuM/GP3DT

matmarchand avatar May 12 '25 18:05 matmarchand

@eringram and I discussed some of the problems today on a call. Recap:

  1. There potentially is some broader work to be done in RealityModelTileTree.ts / RealityDataSourceTilesetUrlImpl.ts to support handling relative URLs in relation to tileset json files and where they reside.
  2. However, in this Google Photorealistic 3D Tiles case, it looks like we actually need to better handle a leading / in a tile's content URI. Such a leading / should mean "prepend the site's base url without any trailing subdirectories." In the case of these Google tiles, the subdirectories are included in the content URI. I coded up a quick version of handling this properly, and it solves that problem.
  3. We then move on to the problem of getting 400 errors (bad request). We theorize this is because nested .json files can have new session parameters --- those parameters likely need to apply to all of their children. The way the code works, it only propagates parameters from the root URL, not any subsequent URLs.

Number 2 is solved on a local branch but could use some cleanup.

Number 3 is the trickiest thing to solve. @andremig-bentley @markschlosseratbentley and @eringram are going to meet tomorrow to discuss in more detail.

markschlosseratbentley avatar May 13 '25 19:05 markschlosseratbentley

@markschlosseratbentley Thanks for the update.

My understanding is that both 1) and 3) are the same in the sense that they need to be "in relation to tileset json files and where they reside." which includes their query params that could be inherited from one of their parent.

For example:

  1. domain.com/root.json?key=x" |_ reference: "ref.json?session=123" ==> we need propagate 'key" from parent 1) : "domain.com/ref.json?session=123&key=x"

  2. domain.com/ref.json?session=123&key=x" |_ reference: "nestedRef.gdb" => we need to propagate 'key' and 'session' from parent 2) : "domain.com/nestedRef.gdb?session=123&key=x"

  3. domain.com/nestedRef.gdb?session=123&key=x

matmarchand avatar May 13 '25 20:05 matmarchand

Thanks @matmarchand for the clear examples.

@markschlosseratbentley I made a quick test on my own branch where I store all query params from tile URLs for ending in "json" in RealityDataSourceTilesetUrlImpl._searchParams, which lets the Google Photorealistic 3D Tiles load correctly in the test case you provided, because all the tiles after the root receive the key and session params.

However, this is not a complete solution because you could have the following case:

"domain.com/root.json?key=x" |_ reference: "ref_1.json?session=123" ==> we need propagate 'key" from parent 1) : "domain.com/ref_1.json?session=123&key=x" |____ |_ reference: "nestedRef.gdb" => we need to propagate 'key' and 'session' from parent 2) : "domain.com/nestedRef.gdb?session=123&key=x" |_ reference: "ref_2.json?session=456" ==> we need propagate 'key" from parent 1) : "domain.com/ref_2.json?session=456&key=x" |____ |_ reference: "nestedRef.gdb" => we need to propagate 'key' and 'session' from parent 2) : "domain.com/nestedRef.gdb?session=456&key=x"

My code would either store both session values 123 and 456, or one would overwrite the other, as both will go in RealityDataSourceTilesetUrlImpl._searchParams (speculating, I haven't written a test for this). This is a first step, and I think we need to figure out how to correctly pass the params down to only tiles that need them

eringram avatar May 13 '25 21:05 eringram

@eringram Thanks for the commit to a test branch - it looks promising. I went ahead and merged your changes into a working branch titled google-3d-tiles where we can all collaborate. I suggest we checkout + work on that branch going forward.

This now also includes a commit by me where I solve the baseURL issue with preceding slashes in a generic way.

Entire diff of google-3d-tiles with master found here.

markschlosseratbentley avatar May 14 '25 12:05 markschlosseratbentley

@eringram @andremig-bentley @markschlosseratbentley met today about this topic.

We have a temporary solution put together that loads Google Photorealistic 3D Tiles, but there is more work to make this "real." See the end of this comment for a screenshot of NYC / central park in iTwin.js display-test-app.

Status:

  • The way we fixed propagation of search params is a temporary solution that does not truly take into account the tile tree structure. We need to do this better.
  • Loading the Google Photorealistic 3D tileset in iTwin.js resulted in a huge usage of memory and loading of way too much detail (inappropriate for the given zoom level). We diagnosed this and found that it is caused by the geometricError of the tileset not being considered by iTwin.js. If we hack iTwin.js to consider this value, it loads much smoother and with less memory overhead. We need to make this not a hack.

@eringram will:

  • [x] compare the way these load in iTwin.js vs. Cesium.js.

@andremig-bentley will collaborate with @eringram to:

  • [x] write some unit tests for the query parameters to verify our current temporary solution does not work in other cases.
  • [ ] look at a way to get the query parameters loading in the most conformant way rather than our current temporary solution.

@markschlosseratbentley will:

  • [x] look at how to make iTwin.js correctly use the geometricError specified in the Google Photorealistic 3D Tiles. This seems like it will resolve major loading and memory issues we observed.

Image

markschlosseratbentley avatar May 14 '25 15:05 markschlosseratbentley

* Loading the Google Photorealistic 3D tileset in iTwin.js resulted in a huge usage of memory and loading of way too much detail (inappropriate for the given zoom level).  We diagnosed this and found that it is caused by the `geometricError` of the tileset not being considered by iTwin.js.  If we hack iTwin.js to consider this value, it loads _much_ _smoother_ and with less memory overhead.  We need to make this not a hack.

I added usesGeometricError for this purpose as part of my PR to support the Google Photorealistic 3d Tiles PoC. Is that what you're using? Did you try it and find it unsatisfactory? By default it uses the same default maximumScreenSpaceError as CesiumJS, which was appropriate for the G3DTs.

pmconne avatar May 14 '25 15:05 pmconne

I added usesGeometricError for this purpose. .... Is that what you're using? Did you try it and find it unsatisfactory?

Yes, we are using that. It solves many issues. The issue is on our end (the caller side) - the way we are setting it needs a bit more work - our enabling of that flag is hacked in place deeper in the code that it should be, and needs to be done correctly at the API level.

markschlosseratbentley avatar May 14 '25 15:05 markschlosseratbentley

I just pushed some more changes to the google-3d-tiles branch.

  • display-test-app UI is now handled better for G3DT (WIP). See screenshot at bottom of this comment. From DTA settings dialog, we can disable Background Map, then enable Google Photorealistic 3D Tiles. BGMap / G3DT cannot be enabled at the same time (need to disable one before enabling the other).
  • I made a new RealityDataSource implementation for G3DT tilesets in file RealityDataSourceG3DTImpl.ts. This seems necessary so we can specify usesGeometricError on this implementation and only this implementation. Furthermore, we can fix up the handling of the G3DT tileset in this class without potentially causing regressions in RealityDataSourceTilesetUrlImpl.ts where processing of many other tilesets happens. Perhaps once we get past the initial development stage we can converge these instances somehow...

Can see entire diff here (click then scroll down): https://github.com/iTwin/itwinjs-core/compare/master...google-3d-tiles?diff=unified&w

Image

markschlosseratbentley avatar May 15 '25 13:05 markschlosseratbentley

By the way, iTwin.js navigation tools seem to work fine with this approach. You can even zoom way out and see the globe and spin it around.

markschlosseratbentley avatar May 15 '25 13:05 markschlosseratbentley

Draft PR: https://github.com/iTwin/itwinjs-core/pull/8104 fyi @pmconne, as discussed in the status call earlier today.

markschlosseratbentley avatar May 15 '25 19:05 markschlosseratbentley

Here's an example visual comparison of how these look in iTwin.js and CesiumJS to make sure we're loading similarly:

CesiumJS Sandcastle with tile bounding volumes on

Image

iTwin.js DTA with reality tiles bounding boxes on

Image

@markschlosseratbentley this made me realize we also need to add the data attribution when G3DT are on in iTwin.js. Worth looking at Michel's Google Maps PR that includes it for the map layer

eringram avatar May 15 '25 20:05 eringram

TODO

GP3DT attribution is the most complicated remaining part.

  • [ ] Finish the draft PR here: https://github.com/iTwin/itwinjs-core/pull/8104
    • [x] Decide whether to keep the GP3DT reality data source implementation separate or combine it with the TilesetUrl implementation to have a single implementation. See more comments below.
    • [x] We need to add copyright attribution as required by Google. See more comments below.
  • [ ] Test via Image Tests -- especially if we merge the GP3DT implementation into the TilesetUrl implementation.

GP3DT vs. TilesetUrl implementation

Is the separate reality data source implementation for GP3DT okay for now? We can keep the new API alpha for easy later removal. Or do we need to, right now, merge the GP3DT implementation into the TilesetUrl implementation to have one unified reality data source implementation?

GP3DT attribution

@eringram pointed out @mdastous-bentley's PR here added attribution for Google Maps in the past. Reading through the comments, it looks like a routine titled addLogoCards was utilized. According to this comment in RealityModelTileTree.ts, addLogoCards is deprecated in 5.0 and we should use addAttributions instead. Looking through the code this morning, this may be difficult. Here are details from Google about where to find the copyright attribution you need to display. From that link: "You must aggregate, sort, and display in a line, all attributions for displayed tiles; usually along the bottom of the rendering. For example, you can find the data attributions in a glTF tile by looking under asset, copyright." Doing this kind of complicated operation makes me lean toward keeping the GP3DT reality data source separate from the TilesetUrl one for now. Google 2D Maps in iTwin.js achieves this through GoogleMapsImageryProvider.ts, doing a separate viewport info request for all tiles and adding them all up. We have no imagery provider, just the reality data source tile tree parsing stuff. Do we even have information about visible tiles?

markschlosseratbentley avatar May 19 '25 12:05 markschlosseratbentley

@markschlosseratbentley is working on adding code to RealityTileLoader.ts which finds and stores attribution information for each GP3DT tile.

@eringram is working on adding code to display the attribution information for each visible tile using addAttributions() in RealityModelTileTree.ts.

These two bits of work can converge into a final solution hopefully.

markschlosseratbentley avatar May 19 '25 18:05 markschlosseratbentley

I am working on handling auth in a proper manner.

@eringram and I made progress yesterday with providing attribution of the GP3DT tiles.

markschlosseratbentley avatar May 20 '25 13:05 markschlosseratbentley

Here's an example visual comparison of how these look in iTwin.js and CesiumJS to make sure we're loading similarly:

@eringram Nice to see that. Can we have a comparison with a more tilted view; toward the horizon? I see that the top most tiles have lower resolution in CesiumJS and not in iTwin.js.

matmarchand avatar May 20 '25 14:05 matmarchand

In our draft PR, the attribution UI works the following way.

The white Google logo shows up in the lower left corner if GP3DT tiles exist in the view:

Image

Once you click on the iTwin.js icon (where we have other attribution stuff), a dialog pops up showing a list of data attribution sources:

Image

This seems roughly similar to the way Cesium.js is doing it in a sandcastle which enables GP3DT. @eringram has a link to that sandcastle. There, a "Data attribution" link shows up in the lower left hand corner which you can click which brings up a similar popup with a list like the one shown above.

In some Cesium.js tutorial screenshots, though, there is a list of data sources showing up in the view itself in the lower left hand corner (no clicking necessary):

Image

Is this the way it used to work in Cesium.js, or...?

Questions for this implementation in iTwin.js:

  • Do we need to make it clearer how to view the attributions? (Add a link in the lower titled "Data attributions" as can be seen in the Cesium sandcastle).
  • OR, instead of a pop-up UI, do we need to display all of these attributions on-screen in the lower left corner?

@ggetz Do you have any feedback?

cc @eringram

markschlosseratbentley avatar May 20 '25 19:05 markschlosseratbentley

Hi @markschlosseratbentley,

Google's attribution requirements and what we should be doing to best comply with them are described in detail in this issue. The TLDR is:

  • We currently do not display all Google P3DT credits on screen, except for the Google logo itself.
  • We do turn them on for screenshots or when capturing video, per our interpretation of the usage requirements. (We have a flag in CesiumJS which can toggle this behavior on and off.)
  • We are considering always showing the credits on screen for Google P3DT to best help users remain in compliance with Google's terms of service.

CC @LisaBosCesium for awareness.

Re: How to view attributions in general–

  • Generally, how we manage data attributions is modeled on recommendations from other geospatial runtimes, such as Esri ArcGIS.
  • We manage attribution location though a flag on each credit: A credit can be shown in "the lightbox" (our name for that dialogue window), or on screen in the lower left corner.
  • The "Data attributions" link toggles the lightbox. Having this be a separate link from the logos is an established practice, but not a struct requirement AFAIK. But because attributions are often determined by data loaded at runtime, they may not map to any displayed logos; In other words, there may be a runtime attribution from user data that should show up in the lightbox but comes from neither Cesium ion or another logo'd source like Google.

ggetz avatar May 20 '25 20:05 ggetz

@ggetz Thanks for the helpful breakdown! @markschlosseratbentley I guess I missed the CesiumJS flag to show them onscreen when we were investigating this. I can look into how viable it is for us to also have a flag like that.

@matmarchand

Can we have a comparison with a more tilted view; toward the horizon? I see that the top most tiles have lower resolution in CesiumJS and not in iTwin.js.

Is this the type of view you meant? Also I just realized those first screenshots had CesiumJS in perspective projection and iTwin.js in orthographic, so that could contribute to the difference-- with both in orthographic, the tiles look even more similar.

CesiumJS perspective projection

Image

CesiumJS orthographic projection

Image

iTwin.js orthographic projection (tile bounds not working properly with perspective projection/camera on)

Image

eringram avatar May 20 '25 22:05 eringram

PR up for review here: https://github.com/iTwin/itwinjs-core/pull/8104

markschlosseratbentley avatar May 22 '25 14:05 markschlosseratbentley