Don't reuse old requestedCameraState.

[chrneumann]

Fixes unexpected camera jumps when transformCameraUpdate is specified and user is panning on maps with terrain (Fixes #4233).

Camera._getTransformForUpdate clones the transform on first call. After that, it is reused on later calls. I am not sure why, maybe only performance reasons? If the transform is cloned on every call (as changed by the PR), the jumps are gone. Could not find any problems yet and the tests run fine. Maybe I overlooked something.

Launch Checklist

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.
• Link to related issues.
• 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.
• Add an entry to CHANGELOG.md under the ## main section.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.48%. Comparing base (d919ba3) to head (58d908d).
Report is 21 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4243      +/-   ##
==========================================
- Coverage   87.73%   87.48%   -0.25%     
==========================================
  Files         242      242              
  Lines       33081    33077       -4     
  Branches     2160     2184      +24     
==========================================
- Hits        29023    28937      -86     
- Misses       3088     3149      +61     
- Partials      970      991      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HarelM]

All tests are passing, meaning who ever added this code didn't add a test to make sure this behavior is kept.
I would advise to dig a bit into the history of this to see if your can understand why this code was added...

[chrneumann]

@Pessimistress Seems to be your code, do you remember why you added this and if it's safe to remove?

[chrneumann]

Okay, I'm still not sure why requestedCameraState is kept, but Camera.easeTo seems to ignore it at some points and instead uses the original transform (that is not yet updated). So it uses states of two different transforms to calculate things, which seems to lead to this (and other?) bugs. Needs some more tests.

[chrneumann]

As far as I understand it correctly, the only reason for requestedCameraState is it's use in the event handlers. Normally it would be enough to call camera._getTransformForUpdate, change the returned transform, and after that call camera._applyUpdatedTransform with the changed transform. But as the returned transform (from _getTransformForUpdate) can't be passed directly to the event handlers (they used to accessed the transform directly through the passed map), the TransformProvider solution was used together with requestedCameraState. Still, I don't see a need to keep the requestedCameraState after the _applyUpdatedTransform. There should be no access to the requestedCameraState outside a _getTransformForUpdate / _applyUpdatedTransform pair. After the _applyUpdatedTransform, the requstedCameraState might differ from the transform, which I think leads to these subtle bugs.

Still not 100 percent sure, but this is as far I can get without feedback or some real world usage examples (could not find any at all). It's not even used in react-map-gl yet I think.

Added a commit to delete the requestedCameraState after _applyUpdatedTransform, not after the moveend event.

[HarelM]

@Pessimistress @birkskyum can you help here?
We are struggling to understand the original reason for writing the code the way it was written...

[HarelM]

@chrneumann can you add a test to simulate the bugs you experience? This way at least we'd know if we change the solution that the original issue is still fixed.

[Pessimistress]

Your change will break the transformCameraUpdate feature for the React use case.

In short, only _requestedCameraState gets updated with the user input, and transform will be updated with an external state store (updates may be batched and asynchronous). If the terrain interaction is experiencing issues, it's likely because some changes are applied to transform directly, bypassing _getTransformForUpdate:

maplibre-gl-js/src/ui/camera.ts

Lines 1073 to 1089 in a3f67eb

	_updateElevation(k: number) {
	this.transform.minElevationForCurrentTile = this.terrain.getMinTileElevationForLngLatZoom(this._elevationCenter, this.transform.tileZoom);
	const elevation = this.terrain.getElevationForLngLatZoom(this._elevationCenter, this.transform.tileZoom);
	// target terrain updated during flight, slowly move camera to new height
	if (k < 1 && elevation !== this._elevationTarget) {
	const pitch1 = this._elevationTarget - this._elevationStart;
	const pitch2 = (elevation - (pitch1 * k + this._elevationStart)) / (1 - k);
	this._elevationStart += k * (pitch1 - pitch2);
	this._elevationTarget = elevation;
	}
	this.transform.elevation = interpolates.number(this._elevationStart, this._elevationTarget, k);
	}
	_finalizeElevation() {
	this._elevationFreeze = false;
	this.transform.recalculateZoom(this.terrain);
	}

[chrneumann]

@Pessimistress Do you have an example/test of your use case or how this breaks it?

[Pessimistress]

Do you have an example/test of your use case

https://codepen.io/Pessimistress/pen/xxNPvVr

I apologize for not adding a unit test for this use case. I can open a PR if that helps. (I may need some guidance on how to simulate user input)

[chrneumann]

https://codepen.io/Pessimistress/pen/xxNPvVr

Thanks, that makes it clearer. But the example seems to have an issue: it's not possible to drag pan the map while the map is still moving because of inertia of a last drag pan. Can't reproduce the problem without the transformCameraUpdate.

[HarelM]

@chrneumann can you please provide test that reproduces the issue that this PR solves?

We need two tests here basically:

1. A test that simulates how the react library is using this API or something similar
2. A test that simulates the camera jump as described here: transformCameraUpdate leads to unexpected camera jumps when targetting terrain #4233

This requires a test from each one of you so that when both tests are fixed we'd know we can merge the change.

If there's a need for me to create a branch in this repo to facilitate for this work please let me know.

[chrneumann]

2. A test that simulates the camera jump as described here: [transformCameraUpdate leads to unexpected camera jumps when targetting terrain #4233](https://github.com/maplibre/maplibre-gl-js/issues/4233)

See #4299.

[chrneumann]

Found an other way to fix the issue in #4299. Still, a use case / test for the React use case is missing.