-
-
Notifications
You must be signed in to change notification settings - Fork 932
Fix get camera altitude #6585
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: main
Are you sure you want to change the base?
Fix get camera altitude #6585
Conversation
|
Thanks for taking the time to open this PR! |
You are very welcome!
Do you maybe know who has expertise in the vertical perspective transform and the globe transform that we could ping for looking at this? It's a bit much to sink my teeth in 😅
Good point, I agree. |
|
@kubapelc probably knows best. |
|
Hi, it has been some time since I last delved into transforms in MapLibre, but I think your fix is correct. |
| this._mercatorTransform.apply(this, true, this.isGlobeRendering); | ||
| this._helper._nearZ = this._mercatorTransform.nearZ; | ||
| this._helper._farZ = this._mercatorTransform.farZ; | ||
| this._helper._pixelPerMeter = mercatorZfromAltitude(1, this.center.lat) * this.worldSize; |
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.
Globe transform should mostly be a wrapper around mercator and vertical presense.
So I would expect this call to be something like:
this._helper._pixelPerMeter = this.currentTransform.pixelPerMeter
Or something similar, can you please check if this is possible?
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.
I tend to think that placing this logic inside the calcMatrices of the helper will solve this issue as the code is the same for both vertical perspective and mercator transforms.
I'm guessing this it's also the right place since the helper is "responsible" for this variable and should update it as needed.
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.
I moved the calculation to the TransformHelper as you suggested.
There is a slight functional difference with my change: MercatorTransform used to recalculate this._helper._pixelPerMeter when this.helper._width is possibly nullable (or 0), but now it is only re-calculated when both _width and _height are defined.
|
Hi there, is there any plan when this fix can make it into maplibre? |
|
I'm not sure, the status of this PR is draft, I'll be happy to review it again after the fixes are made and the status changes to ready for review. |
|
Hi @HarelM Apologies for the inactivity. I will pick it up today! |
…lcMatrices NOTE: MercatorTransform used to recalculate this._helper._pixelPerMeter when `this.helper._width` is possibly nullable (or 0), but now it is only re-calculated when both _width and _height are defined.
|
My fix breaks the same test locally as on CI: |
|
I'm guessing this needs to be addressed then, shouldn't it? |
Example stepping through failing test `Applies options.opacityWhenCovered when marker is covered by globe with terrain disabled or enabled`
I agree! I have been looking at it, trying to conclude if the failing test was a false positive. I recreated the test as an example (and pushed it 👉 0c41471) that I could step through so I could get a bit more of a visual understanding. Before fix Screen.Recording.2025-11-14.at.09.57.50.movIn After fix When we add our fix, we get the following result: Screen.Recording.2025-11-14.at.10.05.34.mov
This results in the following calculation for the failing expectation: // 674396.1640481714
const metersToCenter = -this._offset.y / map.transform.pixelsPerMeter;
// 0
const elevationToCenter =
Math.sin((map.getPitch() * Math.PI) / 180) * metersToCenter;
// 0.9
const terrainDistanceCenter = map.terrain.depthAtPoint(
new Point(this._pos.x, this._pos.y - this._offset.y)
);
// 0.9998595004323604
const markerDistanceCenter = map.transform.lngLatToCameraDepth(
this._lngLat,
elevation + elevationToCenter
);
// true = 0.9998595004323604 > 0.006
const centerIsInvisible = markerDistanceCenter - terrainDistanceCenter > forgiveness; |
|
The test makes use of a mock for map.terrain = {
pointCoordinate: () => null,
getElevationForLngLatZoom: () => 1000,
getMinTileElevationForLngLatZoom: () => 0,
getFramebuffer: () => ({}),
getCoordsTexture: () => ({}),
depthAtPoint: () => .9,
tileManager: {
update: () => {},
getRenderableTiles: () => [],
anyTilesAfterTime: () => false
}
}My assumption would be is that if I mock Screen.Recording.2025-11-14.at.10.29.30.mov |
|
I think the question is around what this test is testing and if this PR didn't broke it: |
I think what I found out is that the terrain occlusion of the markers in globe transform was broken before and that the succeeding test was a false positive. But I don't think my fix is correct either, as I would expect terrain with 0 elevation to return the same results as a globe without terrain. |
|
That's interesting, good thing we have these tests 😀 |
Fixes #6584
map.transform.getCameraAltitude()returnsNaNfor both globe and vertical-perspective projection due topixelsPerMeterbeing undefined.This PR initializes
this._helper._pixelPerMeterjust asMercatorTransformdoes it, I am keeping this PR in draft because I am not super confident this is the correct solution.Launch Checklist
CHANGELOG.mdunder the## mainsection.