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

Add the actual projection matrix to transform #4044

Open
dennemark opened this issue Apr 26, 2024 · 5 comments · May be fixed by #3136
Open

Add the actual projection matrix to transform #4044

dennemark opened this issue Apr 26, 2024 · 5 comments · May be fixed by #3136

Comments

@dennemark
Copy link
Contributor

dennemark commented Apr 26, 2024

User Story

As a developer I can get the projection matrix of the maplibre camera and apply it to a camera of another engine (threejs, bablyonjs, maybe even deckgl).

Rationale

Currently only the world-view-projection matrix is exposed. The projection matrix of transform is not actually a projection matrix, but it is multiplied by the view matrix (correct me if i am wrong).

The projection matrix normally contains nearZ, farZ, fov and aspectRatio.
But the position and rotation of the camera are applied to it, too. (see code link below)

The official examples work around this, by messing with the projection matrices of cameras of threejs and babylonjs and applying a world-view-projection matrix to them. This however can lead to issues with i.e. shadows or line drawings, or click events that need a proper view matrix on a camera (https://forum.babylonjs.com/t/cascaded-shadow-map-are-not-properly-projected-with-camera-freezeprojectionmatrix/49570/14).

Libraries like threebox are copying the projection matrix code of maplibre to get a proper camera position. I have done so, too, now. It gives the same result as the babylonjs example of maplibre. DeckGL is creating its own projection matrix, but I think there were also similar code parts as in maplibre.

But maintaining this will be bothersome, since maplibre might change its calculation of the projection matrix because of the terrain i.e. So it would be nice to expose either the correct projection matrix and give the current projMatrix a new name or it would be nice to expose another property on a transform. The assignment should be added after this line:

m[9] = offset.y * 2 / this.height;

Normally adding 2-3 lines would already fix this. If we agree on naming, I could create a PR and optionally can add another babylonjs example.

Impact

Issues with i.e. shadows or line drawings, or click events in other overlayed engines.

@HarelM
Copy link
Member

HarelM commented Apr 26, 2024

I think there was a PR that started this, the problem is the public API which is hard to define from my point of view.
There is work on the globe branch and @kubapelc has mentioned refactoring the transform class, this might be one of these changes I guess...

@dennemark
Copy link
Contributor Author

Thanks for the reply! Great to hear and makes sense to include it in new version!

Tried to find some discussion on it. But only found some closed PR. Otherwise I would have mentioned it and closed the issue.

Looking forward to the change :)

@HarelM
Copy link
Member

HarelM commented Apr 26, 2024

Can you link to that PR? I'm having a hard time finding it...
But in general, all I remember is that closed PR.

@birkskyum
Copy link
Member

@birkskyum birkskyum linked a pull request Apr 27, 2024 that will close this issue
8 tasks
@HarelM
Copy link
Member

HarelM commented Apr 27, 2024

Thanks @birkskyum! Although not exactly the same thing, the linked PR is related to my concern about public API.

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 a pull request may close this issue.

3 participants