-
Notifications
You must be signed in to change notification settings - Fork 221
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
base: master
Are you sure you want to change the base?
Conversation
…tion. redo UI in DTA.
Thanks for your effort on this. 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. |
Doing everything in We could just put all of this code in I also hesitate to throw all of this into 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 Edit: there is also a concern that altering the core logic of |
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 |
Agree. |
@markschlosseratbentley I added attributions ordered by number of occurrences. If there are copyright properties in the tiles but the tree provider is not I also started working on adding the logo as a decorator on top of the view itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good PR, some nits.
@pmconne By the way, we noticed a side effect here from your recent maximumScreenSpaceError-related PR. Previously, we set This might not be an issue as it didn't show up in the reality data cases in ImageTests, and if you set Max SSE change was added in 1e74630, so I was able to confirm it still defaulted to 16 in 840bdfa. |
Please fix it to default to 16 again. |
@aruniverse please revisit your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work
apiKey: string; | ||
getAuthToken?: never; | ||
/** If true, the data attributions/copyrights from the Google 3D Tiles will be displayed on screen. The Google Maps logo will always be displayed. Defaults to `true`. */ | ||
showCreditsOnScreen?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isnt allowing users to opt out of this a liability for us?
when would this be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We followed the pattern discussed in this comment: #8036 (comment)
Tagging @ggetz as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ggetz Any feedback / thoughts if a user should be able to disable this feature? (showCreditsOnScreen
)
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, and provides a public API to make them easier to attach.
Google3dTilesProvider
This PR adds a
Google3dTilesProvider
class that implements theRealityDataSourceProvider
interface. You can register the provider it withIModelApp.realityDataSourceProviders.register()
, and then attach a reality model viaDisplayStyleState.attachRealityModel()
with yourGoogle3dTilesProvider
name in therdSourceKey
. The following example shows this:RealityDataSourceGoogle3dTilesImpl
It also adds a new
RealityDataSourceGoogle3dTilesImpl
implementation ofRealityDataSource
which is used by theGoogle3dTilesProvider
to request and process Google Photorealistic 3D Tiles. ThisRealityDataSourceGoogle3dTilesImpl
implementation does the following:usesGeometricError
astrue
which tells iTwin.js to use the geometric errors found inside the Google 3D tileset, which is crucial for good performance.maximumScreenSpaceError
to 16RealityDataSourceTilesetUrlImpl
did not handle properly.IModelApp.realityDataFormatRegistry
.GoogleMapsDecorator
Other changes:
GoogleMapsDecorator.ts
frommap-layers-formats
to thecore-frontend
package, to use the decorator to display the logo in the bottom left of the viewport when the Google Photorealistic 3d Tiles are visiblecopyright
property when reading glTF files, which is used to display attributions in accordance with Google's policyshowCreditsOnScreen
flag when you create aGoogle3dTilesProvider
(the flag istrue
by default)Screenshots
Testing
RealityDataSourceGoogle3dTilesImpl
Google3dTilesProvider
, for itsaddAttributions
anddecorate
methodsRealityDataSource
from aGoogle3dTilesProvider
You can also test this implementation using
display-test-app
(DTA) after setting theIMJS_GOOGLE_3D_TILES_KEY
environment variable to a proper value.Enable the Google Photorealistic 3D Tiles in DTA by toggling off Background Map and toggling on Google Photorealistic 3D Tiles in the view settings dialog:
TODO
G3DT
for Google Photorealistic 3d Tiles. Should we useGP3DT
instead? I have seen both forms used. We have now decided to useGP3DT
and this change has been pushed.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 theTilesetUrl
implementation does not handle.copyright
is currently in theTile
class - is this okay?gp3dtKey
ontoTileAdmin
, look into implementing something similar toMapLayerFormatRegistry.ts
.RealityDataSourceGP3DTProvider
and allow users to pass inapiKey
andgetAuthToken
functionGoogleMapsDecorator
Future (later PRs):