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

Incorrect custom layer render methods argument args.defaultProjectionData.mainMatrix value using globe projection with high zoom level #5117

Closed
sebstryczek opened this issue Nov 25, 2024 · 11 comments · Fixed by #5139
Labels
bug Something isn't working PR is more than welcomed Extra attention is needed

Comments

@sebstryczek
Copy link

maplibre-gl-js version: 5.0.0-pre.7

browser: Chrome Version 131.0.6778.86 (Official Build) (x86_64) (MacOS, Intel)

Steps to Trigger Behavior

  1. Use test/examples/globe-3d-model.html
  2. Zoom to 3D model until globe projection change to mercator.

Link to Demonstration

https://jsbin.com/bedahot/edit?html,output

Expected Behavior

3D model should be visible on all zoom levels.

Actual Behavior

3D model disappear on zoom level 12 (when projection is changed to mercator).

Screen.Recording.2024-11-25.at.15.25.01.mov

I was able to make it work using:

// const defaultProjectionData = args.defaultProjectionData;
const defaultProjectionData = args.defaultProjectionData.projectionTransition === 1
  ? this.map.transform.getProjectionDataForCustomLayer()
  : this.map.transform._mercatorTransform.getProjectionDataForCustomLayer();

const matrix = defaultProjectionData.mainMatrix;
@HarelM
Copy link
Collaborator

HarelM commented Nov 26, 2024

When using the main branch and looking at the add-3d-model.html file or globe-3d-model.html I don't see any issues.
Please check the code there and see if you can spot the changes.
I see some z-fighting in your example even before the model disappears.

@HarelM HarelM added the need more info Further information is requested label Nov 26, 2024
@sebstryczek
Copy link
Author

sebstryczek commented Nov 27, 2024

Hi! Thank you for your response!

So I just did a quick check on latest main and it is still working like that. Here are some videos from the globe-3d-model.html (add-3d-model.html worked perfect even before).

Here are some videos.

  1. Nothing modified (only added maptiler key).
Screen.Recording.2024-11-27.at.15.54.44.mov
  1. Changed object scale to present the issue better.
Screen.Recording.2024-11-27.at.15.58.24.mov

When you looked at globe-3d-model.html, did you try zooming closer to the object?

@HarelM
Copy link
Collaborator

HarelM commented Nov 28, 2024

I'm not a three.js expert, so I'm guessing you'll need to dig into it and understand what doesn't work correctly...

@HarelM HarelM added bug Something isn't working PR is more than welcomed Extra attention is needed and removed need more info Further information is requested labels Nov 28, 2024
@sebstryczek
Copy link
Author

sebstryczek commented Nov 28, 2024

Maybe I am wrong (I didn't go very deep to the source code) but let me share two more examples.

  1. Projection set to globe, but zoom is high enough to make args.defaultProjectionData.projectionTransition be 0 - so actually we see the mercator.
    https://jsbin.com/weyiful/1/edit?html,console,output
    args.defaultProjectionData.mainMatrix is equal to this.map.transform.getProjectionDataForCustomLayer().mainMatrix.
    Additionally: this.map.transform._mercatorTransform.getProjectionDataForCustomLayer().mainMatrix is
[9000127.451114923, 0, 0, 0, 0, -6291456, 0, 0, 0, 0, -2153263.090301004, -2097152, -8224663.969749846, 3807945.9490339337, 1250.6652842809367, 1251]
  1. Projection set to mercator, everything else: the same as in example above.
    https://jsbin.com/zibetat/edit?html,console,output
    this.map.transform._mercatorTransform doesn't exists but:
    args.defaultProjectionData.mainMatrix is equal to this.map.transform.getProjectionDataForCustomLayer().mainMatrix.
    And its value is:
[9000127.451114923, 0, 0, 0, 0, -6291456, 0, 0, 0, 0, -2153263.090301004, -2097152, -8224663.969749846, 3807945.9490339337, 1250.6652842809367, 1251]

So I feel like it is not directly related to three.js but rather with value passed via render method.

@HarelM
Copy link
Collaborator

HarelM commented Nov 28, 2024

What you detailed above is exactly the implementation details.
When globe switches the view from globe to mercator it sends the mercator protection matrix (it holds a mercator transform internally). When you sat protection to mercator it doesn't have an "internal" transform.
If this transform isn't correct or problematic that's a different story...

@kubapelc
Copy link
Collaborator

I think I've managed to fix this, it is a bug in getProjectionDataForCustomLayer in globe_transform.ts, I'll post a PR.

@HarelM
Copy link
Collaborator

HarelM commented Dec 2, 2024

I'm not sure if this is related to this issue or not, but it seems that in main the following example is broken:
test/examples/globe-custom-tiles.html
Image

Don't we have a render test to cover this use case?

@kubapelc
Copy link
Collaborator

kubapelc commented Dec 2, 2024

globe-custom-tiles.html is broken because the transform's API changed slightly, it is an issue inside the example's code. I've already fixed it locally and will include it in the PR that fixes this issue. It is fixed by replacing const projectionData = map.transform.getProjectionData({overscaledTileID: tileID}); with const projectionData = map.transform.getProjectionData({overscaledTileID: tileID, applyGlobeMatrix: true});.

@HarelM
Copy link
Collaborator

HarelM commented Dec 2, 2024

Can't we use the args for this instead of using an "internal" API?

@kubapelc
Copy link
Collaborator

kubapelc commented Dec 3, 2024

We can add any single function or API to args, but example is a showcase of a pretty complex custom layer and it only needs one specific function to do it, real world layers might need a lot more of the transform exposed. I would say that whatever is exposed "properly" through args should be sufficient for the simple layers, since we have no way of predicting what more advanced layers might need.

@HarelM
Copy link
Collaborator

HarelM commented Dec 3, 2024

I think it will be better not to showcase an example that uses an "intenal" API, it's true that people might need it, but I would like to avoid "promoting" it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working PR is more than welcomed Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants