Fix distance-to-center filtering of symbols when terrain is enabled

[akoylasar]

This PR is a fix for #11455.
One other thing that was discovered during debugging was that transform.cameraWorldSize is returning an incorrect value that had gone unnoticed. The fix for this although trivial has been left out to avoid breaking the fog visuals for the existing customers. This will probably be addressed by a separate PR once it's been communicated.

Before:

before.mov

After:

after.mov

cc @mapbox/gl-native

TODO:

• add a render test to catch such regressions in future.
• add a diagram/graph to communicate why the solution here is "more correct".

Launch Checklist

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes
• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page
• tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
• tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix the disappearing symbols when center-to-distance is used along with terrain.</changelog>

[mpulkki-mapbox]

This was clearly a bug that a mercator coordinate was used instead of a latitude one. Would it be better to convert the y-coordinate into latitude value inside the function rather than expecting it as a function parameter? Another option would be to change elevationMeters to elevationMerc or something.

[akoylasar]

exactly, camera.position is in mercator coordinates.

[mpulkki-mapbox]

I don't see any bugs in the implementation other than mercator coordinate being used instead of a latitude one. This change introduced here means that the camera world size is computed relative to camera center (which is exactly what the default worldSize does) which might vary a lot especially in mountainous areas. I haven't tried this out but I would think that the fog visuals now changes as a function of the centre altitude which might not be desirable.

[mpulkki-mapbox]

cc @karimnaaji

[akoylasar]

@mpulkki-mapbox it's not enough to only fix convert mercator to lat/lng. getDistanceToElevation effectively returns the distance of the camera from the center (not camera center) before taking pitch into account. My guess was that originally perhaps this was meant to be calculated as such. On the other hand as I mentioned in the PR description, simply using worldSize instead of cameraWorldSize to address this bug would not alter the behaviour of the fog at all.

[mpulkki-mapbox]

getDistanceToElevation is pretty much just a ray-plane intersection test where the physical camera position is the ray origin and z = averageElevation is the target plane. Changing averageElevation to centerAltitude is a fundamental change to the function and I'm interested in hearing why that's required to fix the original issue. Could you please update the ticket description to better describe what was actually incorrect in the original return value of the function? :)

[akoylasar]

@mpulkki-mapbox exactly, it wasn't clear to me why we use the averageElevation instead of centerAltitude. I didn't try this out as I was fairly certain it shouldn't cause any break. But now I see from the video you provided that the fog indeed is jittering. The jittering is likely to occur anyways but is less noticeable because we do sampling for the camera center. For the map center however there's no such sampling. Interestingly though for the symbols I what's implemented here is still "correct" since we don't really care about the elevation there anyways. Thanks again for taking time and carefully catching this one!
That said I am going change the code back to use averageElevation. The change here then:

1. Fixes the missing conversion (mercator y to lat).
2. Uses worldSize for the calculateDistanceTileData to address the bug at hand.

[rreusser]

Hi 👋 This PR covers the reason for averageElevation: #10621. The development of averageElevation was probably the most challenging and frustrating aspect of getting fog to work acceptably. The fundamental problem was that it's not exactly clear how to scale fog, as it's not tied to a static physical scale. At the same time, the map zoom is also unacceptable for scaling fog. Where 3D is concerned, the zoom—which is used to scale line widths and other style properties—turns out not to work well at all, as it updates discontinuously and changes significantly, depending on what the centerline of the camera intersects.

So the question is what to use in order to determine a suitable scale for fog. If you intersect the camera ray with sea level, then at nonzero pitch and nonzero altitude, the distance to sea level is significantly farther than the distance to the terrain and fog disappears entirely. The effect then is that no matter how you configure fog, it always disappears as conditions change. We wanted to be able to configure it once and have it feel the same no matter where you are in the world.

If you use centerAltitude, then there are two problems: The first is that this only updates on map idle, so as you pan and whenever terrain is present, the scale isn't right and fog usually either obscures everything or disappears entirely. The second is that it's quite jumpy, as it only takes a single point into account (at the center of the frame) which doesn't accurately reflect the overall picture you're looking at.

Our solution was to sample five points, stretched to fit between the bottom of the map and the horizon line. The points are sampled at a rate of about twice per second whenever the map is being redraw. We compute the elevation of those five points and from that, the average elevation. Finally, the average elevation is time-eased so that updates are smooth. During rendering, we intersect the camera ray with a plane at that average elevation in order to scale the fog.

The image below shows a debug view of the five sampled elevations (in meters), from which the average elevation plane is determined.

I know that's a bit tricky, but I hope that at least explains the rationale for this complexity. Please let me know if any of the motivation is unclear.

[rreusser]

Here's a quick sketch of a side-view representation of what's going on. Black is the camera frustum. The blue X is what happens if you intersect the camera centerline with sea level: it's too far away and fog disappears. The red X is what happens if you intersect only the centerline with terrain: it's very sensitive to bumpiness of terrain. The green X represents what happens if you average five points and intersect the camera centerline with a plane at that altitude: it's a better representation of what you're looking at which isn't so sensitive to bumps.

[akoylasar]

@rreusser Thanks a lot for this detailed explanation! It wasn't clear to me that when terrain is enabled we're actually recalculating the center accordingly. Without terrain it's been the other way around AFAIR that we calculate the camera position accordingly to camera rotation and map center.

[rreusser]

Ah, yes, I think the camera moves freely in three dimensions during interaction, not directly in terms of the map center and zoom. But whenever the map idles, it ray-intersects the terrain, recomputes the camera center, and sets the zoom accordingly.

[mpulkki-mapbox]

Unfortunately this fix seems to break the fog computation as I was expecting in one of my earlier comments. Updating the function getDistanceToElevation to use the map center elevation instead of the average elevation causes the fog distance to jump back and forth as center altitude changes. This does not happen in the main branch.

Screen.Recording.2022-11-26.at.21.30.18.mov

[mpulkki-mapbox]

Do we have any render tests for validating the fix? The definition of distance-from-center expression is as follows:

Returns the distance of a symbol instance from the center of the map.
The distance is measured in pixels divided by the height of the map container.

This change might suffer from same problems as the fog when you modified the other function to use worldSize instead of cameraWorldSize. I believe you should modify both scale and center variables to use pixel coordinates of the cameraWorldSize rather than worldSize.

[akoylasar]

Do we have any render tests for validating the fix?

yes indeed it was in todo list above to add a render test.

This change might suffer from same problems as the fog when you modified the other function to use worldSize instead of cameraWorldSize.

In practice I think it's fine to use worldSize and leave the rest unchanged since we don't really care about terrain elevation (we use map center point) and so I also don't think this breaks the contract of the API. The rationale for me here was that we keep it as simple as possible and not involve terrain (cameraWorldSize varies with elevation) in the calculations in the first place. But I am curious why you think using cameraWorldSize and fixing scale and center instead is in practice a better solution?

[mpulkki-mapbox]

You're on a right track here but worldSize works quite the opposite: it can be interpret as a distance between the camera physical location and the map center point. With terrain enabled this distance can vary a lot especially in areas with big elevation differences because by default the camera keeps its altitude relative to sea level. Such changes in the default zoom level are also reflected in the worldSize which could be seen as the fog distance jumping back and forth. For this reason the connection between screen pixels and world pixels is lost making the result of the filter dependant on camera location and terrain elevation.

The "camera zoom" was initially introduced as a zoom level that's computed relative to the sea level in order to solve this very problem. Purpose of the cameraZoom has been changed to describe distance to average visible elevation and a new variable this._seaLevelZoom has been introduced. I think we should consider using either camera zoom or sea level zoom as the fix.

I'm not saying that using the default world size is totally out of question, but there has to be clear understanding and reasoning behind that.

[akoylasar]

Thanks @mpulkki-mapbox for the explanation. Indeed I wasn't aware of some of subtleties involved when terrain is enabled. I decided to only update the center local variable in calculateDistanceTileData to be in the correct coordinate space. This was the root cause. Alternatively wecould also use the this._seaLevelZoom as follows:

const ws = this._seaLevelZoom === null ? this.worldSize : this.tileSize * this.zoomScale(this._seaLevelZoom);
const scale = ws / this.zoomScale(canonical.z);
const unwrappedX = canonical.x + Math.pow(2, canonical.z) * unwrappedTileID.wrap;
const tX = unwrappedX * scale;
const tY = canonical.y * scale;

const center = this.point;
center.x *= ws / this.worldSize;
center.y *= ws / this.worldSize;

both of these are consistent.

[SnailBones]

The change to fog seems to be causing more distant terrain to lose visibility in the fog. Even if we resolve the fog jumpiness pointed out by @mpulkki-mapbox, I think that we need to carefully consider any changes that could cause a visual regression.

[akoylasar]

The change to fog seems to be causing more distant terrain to lose visibility in the fog. Even if we resolve the fog jumpiness pointed out by @mpulkki-mapbox, I think that we need to carefully consider any changes that could cause a visual regression.

@SnailBones the only change that affects the fog is the missing conversion from mercatorY to latitude in free_camera.getDistanceToElevation. This was simply an overlapping issue that could have been addressed separately. I don't think it should be kept buggy simply because the fog visuals have changed. The current fog visuals are the correct ones. The remaining changes here aren't related to the fog but symbols.

[SnailBones]

@SnailBones the only change that affects the fog is the missing conversion from mercatorY to latitude in free_camera.getDistanceToElevation. This was simply an overlapping issue that could have been addressed separately. I don't think it should be kept buggy simply because the fog visuals have changed. The current fog visuals are the correct ones.

Thanks for the explanation @akoylasar! I agree that we should fix this bug, but we should keep in mind that all customers currently using fog have designed their fog for the buggy behavior, and fixing the bug will cause a significant visual change.

Thoughts on adjusting the default fog (setFog()) so that visually it remains similar? (cc @karimnaaji ) It also might be worth reaching out to the design team to make the same update the new default styles. (That's a more complex issue due to different style/GL JS version combos, but we could at least ensure that customers using the latest style and the latest GL JS version won't experience a regression.)

Alternatively, is there another place in the code that we could change to could we change (i.e. multiply fog values by a constant?) to keep the behavior of fog unchanged while still fixing the bug in getDistanceToElevation? This approach would be less elegant and introduce a "magic number", but I think it deserves consideration since the alternative is a regression.

[karimnaaji]

Good catch on this @akoylasar , the fix looks good to me in code.

For the fog visual differences I agree with @SnailBones, it's unfortunate to catch this bug after the facts, but we should be really cautious to not introduce any visual difference with the previous behavior. Users shouldn't have to change their settings to account for something we did incorrectly.

The option to add a small fixed scale factor seems reasonable to keep the previous and current behavior as close as possible. Otherwise, could we get more before/after screenshot comparing main with this PR with various fog distance settings to better understand the impact?

[akoylasar]

@SnailBones the only change that affects the fog is the missing conversion from mercatorY to latitude in free_camera.getDistanceToElevation. This was simply an overlapping issue that could have been addressed separately. I don't think it should be kept buggy simply because the fog visuals have changed. The current fog visuals are the correct ones.

Thanks for the explanation @akoylasar! I agree that we should fix this bug, but we should keep in mind that all customers currently using fog have designed their fog for the buggy behavior, and fixing the bug will cause a significant visual change.

Thoughts on adjusting the default fog (setFog()) so that visually it remains similar? (cc @karimnaaji ) It also might be worth reaching out to the design team to make the same update the new default styles. (That's a more complex issue due to different style/GL JS version combos, but we could at least ensure that customers using the latest style and the latest GL JS version won't experience a regression.)

Alternatively, is there another place in the code that we could change to could we change (i.e. multiply fog values by a constant?) to keep the behavior of fog unchanged while still fixing the bug in getDistanceToElevation? This approach would be less elegant and introduce a "magic number", but I think it deserves consideration since the alternative is a regression.

Thanks @SnailBones! this is of course a very valid point. In my response to @mpulkki-mapbox here I also described an alternate solution that would still fix the bug at hand and leave the fog unchanged. I don't insist on fixing the fog issue in this PR but I think it needs to be handled at some point. Let's get some more feedback before concluding it. cc @mpulkki-mapbox @rreusser @karimnaaji

[SnailBones]

This duplicate function seems potentially confusing. Would it be possible to tweak fog to effectively stay the same, while still applying the conversion correctly?

If that's not possible, thoughts on giving the old function a more specific name, e.g. cameraWorldSizeForFog?

[akoylasar]

Sorry made a blunder in the naming. How about cameraWorldSizeForSymbols instead for the new function just to keep the scope small and avoid updating many files. IMHO it's better to have this in place instead of adding another multiplication factor which can cause more confusion.

[SnailBones]

Not a blocker and looks like you've addressed this in a comment, but IMO changing the name of the old (buggy) function would make it more clear that if we're adding new features that use cameraWorldSize they should use the fixed version.

It looks like it's only used in transform.js so I think the required changes should be small.

[akoylasar]

I meant in many places*. Indeed it's only in transform.js. No strong preference here so I'll update it.

[SnailBones]

Nit: $FlowFixMe suggests a flow error, which I don't think is occurring here. Should we cut it or replace with "FIXME:" ?

Suggested change

	// $FlowFixMe This getter returns the incorrect value. See free_camera.getDistanceToElevation for the rationale.
	// This getter returns the incorrect value. See free_camera.getDistanceToElevation for the rationale.

[akoylasar]

I originally added "TODO" but lint was complaining (too strict perhaps?), I think I kind of abused the $FlowFixMe in this case. Will remove it altogether.

[SnailBones]

Added a few nits but looks good to me, nice fix @akoylasar!