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

Example for using with Mapbox GL JS #435

Closed
wants to merge 6 commits into from

Conversation

xiaxiangfeng
Copy link

add a Example for using with Mapbox GL JS #426

Copy link
Contributor

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

Thank you! I've added a few notes - and do you mind adding comments to the code? Especially the "mapboxExampleCamera". It's not easily clear what it's for or what it does from looking at the code. Is it the case that it needs to be a class like this?

example/mapboxExampleCamera.js Outdated Show resolved Hide resolved
example/mapboxExample.js Outdated Show resolved Hide resolved
example/mapboxExample.js Outdated Show resolved Hide resolved
@xiaxiangfeng
Copy link
Author

Thank you! I've added a few notes - and do you mind adding comments to the code? Especially the "mapboxExampleCamera". It's not easily clear what it's for or what it does from looking at the code. Is it the case that it needs to be a class like this?

You can add comments or modify some code

The absence of comments in mapboxExampleCamera does make it difficult for people to understand. I can add some comments to make it easier to understand

@xiaxiangfeng
Copy link
Author

xiaxiangfeng commented Jan 2, 2024

modify:
1.users provide their own key in the GUI to use the demo
2.changed tileset url
3.Add comments to the core of the code in the mapboxExampleCamera file
4.indentation consistently

Copy link
Contributor

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

Thanks for adding some comments! I've written a few more notes. I think the "updateCamera" portion of the code should be able to be simplified quite a bit. It would be best to use as many built in three.js functions to compute the position and camera matrices as possible so it's easier to follow. Accessing direct matrix elements and keeping copies of different matrices around makes the code more difficult to follow. As does keeping some functions around that are unused.

I'm happy to clean things up a bit afterward but I just want to make sure I understand what's happening since I'm new to MapBox and it's clear to others, as well.

Also a couple comments about the demo:

  • It looks like the camera world matrix is being updated a frame late which is causing pieces of the tileset to load in delayed which causes a flash. We may need to call "camera.updateMatrixWorld" before triggering "tiles.update()" in the render function.
  • When zooming into the 3d tiles model from above parts of the model disappears - it seems like something isn't being updated correctly:
image

example/mapboxExample.js Outdated Show resolved Hide resolved
example/mapboxExample.js Outdated Show resolved Hide resolved
example/mapboxExampleCamera.js Outdated Show resolved Hide resolved
example/mapboxExample.js Show resolved Hide resolved
example/mapboxExampleCamera.js Outdated Show resolved Hide resolved
example/mapboxExampleCamera.js Outdated Show resolved Hide resolved
example/mapboxExampleCamera.js Show resolved Hide resolved
example/mapboxExampleCamera.js Outdated Show resolved Hide resolved
example/mapboxExampleCamera.js Outdated Show resolved Hide resolved
example/mapboxExampleCamera.js Outdated Show resolved Hide resolved
@xiaxiangfeng
Copy link
Author

Thanks for adding some comments! I've written a few more notes. I think the "updateCamera" portion of the code should be able to be simplified quite a bit. It would be best to use as many built in three.js functions to compute the position and camera matrices as possible so it's easier to follow. Accessing direct matrix elements and keeping copies of different matrices around makes the code more difficult to follow. As does keeping some functions around that are unused.

I'm happy to clean things up a bit afterward but I just want to make sure I understand what's happening since I'm new to MapBox and it's clear to others, as well.

Also a couple comments about the demo:

  • It looks like the camera world matrix is being updated a frame late which is causing pieces of the tileset to load in delayed which causes a flash. We may need to call "camera.updateMatrixWorld" before triggering "tiles.update()" in the render function.
  • When zooming into the 3d tiles model from above parts of the model disappears - it seems like something isn't being updated correctly:
image
  1. I added camera.updateMatrixWorld but nothing seems to change
  2. The tileset used now has terrain, it needs to be placed in its correct position, and mapboxgl needs to be turned on for terrain. We can also set tilesGroup.position.z = 1; make the tileset higher than the mapboxgl ground

@gkjohnson
Copy link
Contributor

I added camera.updateMatrixWorld but nothing seems to change

Interesting - once the code is simplified more I can take a look.

The tileset used now has terrain, it needs to be placed in its correct position, and mapboxgl needs to be turned on for terrain. We can also set tilesGroup.position.z = 1; make the tileset higher than the mapboxgl ground

Does this explain why the map plane seems to be moving, though? As you zoom in and out and rotate the camera the portion of the terrain being hidden seems to change.

@xiaxiangfeng
Copy link
Author

I added camera.updateMatrixWorld but nothing seems to change

Interesting - once the code is simplified more I can take a look.

The tileset used now has terrain, it needs to be placed in its correct position, and mapboxgl needs to be turned on for terrain. We can also set tilesGroup.position.z = 1; make the tileset higher than the mapboxgl ground

Does this explain why the map plane seems to be moving, though? As you zoom in and out and rotate the camera the portion of the terrain being hidden seems to change.

When z is increased, there is still a problem, it seems to be related to depth writing, I am not sure

@xiaxiangfeng
Copy link
Author

xiaxiangfeng commented Jan 8, 2024

modify:

  1. Delete the default accessToken
  2. Add camera.updateMatrixWorld
  3. Simplify the code of mapboxExampleCamera
  4. Delete lights
  5. Set tilesGroup.position.z = 1

@gkjohnson
Copy link
Contributor

gkjohnson commented Jan 10, 2024

When z is increased, there is still a problem, it seems to be related to depth writing, I am not sure

I don't think this can be merged until it works correctly and I don't have the bandwidth to debug these issues. I recommend asking at the three.js forum to find help with how to properly integrate three.js with mapbox and extract the camera transform and camera frustum separately. Alternatively we could update the tiles renderer project to be resilient to this kind of matrix but I'd prefer to avoid that and this doesn't fix the raycasting issue. Ideally mapbox would provide a separate view and projection matrix (there was a discussion here but it didn't go anywhere).

For reference it looks like there's MapBoxGL documentation for three.js integration here and threebox provides some integration utilities.


Relating to threebox - it looks like you have have copy and pasted quite a bit of code from the threebox project without attribution. At least from the CameraSync file and other constants. Is this the case? Its your responsibility to properly attribute code and notify projects of where the code has come from if it is not yours.

@xiaxiangfeng
Copy link
Author

xiaxiangfeng commented Jan 10, 2024

When z is increased, there is still a problem, it seems to be related to depth writing, I am not sure

I don't this can be merged until it works correctly and I don't have the bandwidth to debug these issues. I recommend asking at the three.js forum to find help with how to properly integrate three.js with mapbox and extract the camera transform and camera frustum separately. Alternatively we could update the tiles renderer project to be resilient to this kind of matrix but I'd prefer to avoid that and this doesn't fix the raycasting issue. Ideally mapbox would provide a separate view and projection matrix (there was a discussion here but it didn't go anywhere).

For reference it looks like there's MapBoxGL documentation for three.js integration here and threebox provides some integration utilities.

Relating to threebox - it looks like you have have copy and pasted quite a bit of code from the threebox project without attribution. At least from the CameraSync file and other constants. Is this the case? Its your responsibility to properly attribute code and notify projects of where the code has come from if it is not yours.

Okay, I understand the current situation
Sorry for not stating the source of the code
The code in CameraSync file comes from https://github.com/jscastro76/threebox/tree/master I have simplified and modified some content Now I have explained it in the code

@xiaxiangfeng
Copy link
Author

First of all, I would like to thank gkjohnson for his support these days. I will close this PR for now. I feel that it cannot solve the existing problems at the moment. I also hope that someone who encounters a similar situation in the future can provide help or complete this PR.

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.

None yet

2 participants