Skip to content
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

update loader to support google maps 3d tiles #39

Merged
merged 15 commits into from
Mar 14, 2024
Merged

update loader to support google maps 3d tiles #39

merged 15 commits into from
Mar 14, 2024

Conversation

kfarr
Copy link
Contributor

@kfarr kfarr commented Mar 10, 2024

Work in progress, remaining items before merging:

Pre-merge list:

  • @Algorush modify readme docs to add info about new component properties
  • for 3DStreet use case, research how to implement 'reduce draw distance' option -- currently way too many tiles are loaded in the distance that are not visible in this use case (workaround by setting distanceScale to 0.01, see comment below)
  • @Avnerus to review
  • consider adding warning and instructions to docs regarding google policy for default 3d tiles root json load quota Add warning to Readme regarding approval process for use of Google map tiles 3D API #40
  • @Algorush test removing preserveDrawingBuffer

Not resolving for this PR (future):

@kfarr
Copy link
Contributor Author

kfarr commented Mar 10, 2024

We had an issue where way too many tiles are loaded in the distance than are needed for this application. There are not visible as they are mostly occluded by buildings in the scene given the camera placement. For users, the issue can be both the draw area is too big and/or the amount of bandwidth transfer for these tiles is excessive for the use case.

Instead of a radius-based drawing limit, if we just focus on MB of data transferred the distanceScale option addresses this. Testing different values for distanceScale over the same use case of ~45 seconds of usage of the application resulted in the following:

  • 0.01 = 10MB for 45-60 seconds of heavy usage;
  • 0.1 = 20MB;
  • 1.0 = 60MB

Therefore setting distanceScale: 0.01 (for our use case) will fix the throughput issue in the short-term and later we can research drawing only in a specified area.

kfarr and others added 3 commits March 9, 2024 21:37
not implemented, future feature to prevent pausing in inspector / editor
@Avnerus
Copy link
Contributor

Avnerus commented Mar 13, 2024

Thank you for this work. Looks great!
I left some comments and questions in the review.

We had an issue where way too many tiles are loaded in the distance than are needed for this application. There are not visible as they are mostly occluded by buildings in the scene given the camera placement

I think the best bet here would be to implement occlusion culling as in the threedtiles project (service, usage).

This requires some modification to three-loader-3dtiles and also to loaders.gl, to allow a callback for an occlusion service. I might be able to get around to this in the near future, but if you need this now you are welcome to take on this task!

lat: 37.77522354250163;
long: -122.41931773049723;
height: -16.5;
googleApiKey: INSERTYOURKEYHERE;
Copy link
Contributor

Choose a reason for hiding this comment

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

The example demo states that this and also lat/long/height are available parameters but there is no option to insert them via the query string.

Copy link
Contributor Author

@kfarr kfarr Mar 13, 2024

Choose a reason for hiding this comment

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

but there is no option to insert them via the query string.

Correct, the expected behavior in this example is only to place tiles with a long / lat / height specified in the index.html centered at scene origin 0 0 0. Supporting a dynamic, user-specified querystring with lat / long / height was not intended to be part of this example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it.
It's maybe OK since the example is anyway not exposed in the demo showcase, but if it works well we should probably consider adding it in the future and also make the example usable without editing the code. Maybe after we have occlusion culling and teleport controls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

index.js Outdated
this.el.setObject3D('tileset', model);

this.originalCamera = this.camera;
this.el.sceneEl.renderer.preserveDrawingBuffer = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check with Alex, but I'm pretty sure this is not needed and was only included to improve generation of screenshots for our use case. We can remove this and add elsewhere in our application instead.

Copy link
Contributor Author

@kfarr kfarr Mar 14, 2024

Choose a reason for hiding this comment

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

Ok this has been removed. Just to make sure I tested with and without and didn't notice any effect.

@Avnerus Avnerus merged commit 9e7f8bc into nytimes:dev Mar 14, 2024
1 check failed
@Avnerus
Copy link
Contributor

Avnerus commented Mar 14, 2024

Thank you!

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.

3 participants