Fix missing constrainOverride setter in TransformHelper.apply

[larsmaxfield]

Bug

In v5.10.0 transformConstrain is ignored when terrain is enabled. See CodePen example.

Terrain relies on cloning the transform, but cloning does not pass the custom constrain.

Problem

The apply() call in clone() does not set constrain, meaning cloned transforms don't have the (potentially) custom constrain.

If we set the old constrain to the new one, we might actually set an unchanged default constrain. This is a problem when we switch projections because we don't want to set the old default to an unrelated transform. (Mercator's default is different than Globe's, for example).

Proposal

Redesign constrain and transformConstrain:

• Add transformConstrain to ITransformGetters
• Change TransformHelper.constrain() to return transformConstrain if defined, otherwise return defaultConstrain. We do not overwrite constrain() and it cannot be set.
• Apply transformConstrain in TransformHelper.apply(), ensuring all transform clones retain the custom constrain.

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.
• Write tests for all new functionality.
• Add an entry to CHANGELOG.md under the ## main section.

[larsmaxfield]

Switched to draft. Need to check the failing tests before review.

[codecov]

Codecov Report

❌ Patch coverage is 89.58333% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.52%. Comparing base (25d189b) to head (6f2a73d).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/geo/projection/vertical_perspective_transform.ts	57.14%	3 Missing ⚠️
src/geo/projection/globe_transform.ts	71.42%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6642   +/-   ##
=======================================
  Coverage   92.51%   92.52%           
=======================================
  Files         283      283           
  Lines       23389    23401   +12     
  Branches     5068     5071    +3     
=======================================
+ Hits        21639    21652   +13     
+ Misses       1750     1749    -1     

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

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[larsmaxfield]

I originally targeted TransformHelper.apply() to copy the transform constrain:

    public apply(thatI: ITransformGetters, constrain?: boolean, forceOverrideZ?: boolean): void {
        this._constrain = thatI.constrain;

However, this copies thatI.constrain regardless of it being custom or default: if you switch projections from a default Mercator to Globe, the resultant apply() will set the default Mercator constrain to the Globe. That's why those globe tests failed.

We need to set the constrain only if it differs from the default because that indicates it is custom. But we can't get the default from thatI, so we would need to somehow pass it to apply(). I propose instead doing this in each transform's clone() before it calls apply().

[HarelM]

Unfortunately, this apply/clone is too complicated. But it might be that the way we decided to store the constrain is also complicated... IDK...
I would recommend trying to fully understand what's going on there before trying to hack a solution...

[larsmaxfield]

Fair. And if we define "apply" as "apply everything from this transform", then the current implementation isn't doing that because it skips the constrain.

[larsmaxfield]

The design now sets transformConstrain separate from constrain.

constrain is no longer set, but rather returns transformConstrain if defined, otherwise returns defaultTransform.

*This means transformConstrain is a property like renderWorldCopies, rather than something that overwrites constrain.

[larsmaxfield]

Renamed the constrains. Perhaps a bit verbose, but I want to make it clear what is happening with this approach:

• getConstrain: Always call this to get the applicable constrain
• defaultConstrain: The default constrain function defined by each transform (MercatorTransform, etc.)
• transformConstrain: The optional user-specified function that getConstrain calls instead of defaultConstrain if it is defined

*I think I was using "override" incorrectly in my previous comments to describe what transformConstrain does. It doesn't change defaultConstrain, rather it is what getConstrain calls instead of defaultConstrain

[HarelM]

While there might be a different way to solve this, this is probably the most straight forward one.
But there's a need to make the names a lot more clearer in what they do.

[larsmaxfield]

Names updated:

• applyConstrain() to apply the applicable constrain to the center and zoom
• constrainOverride as the optional callback that is used instead of defaultConstrain, if defined

I previously named the override "transformConstrain" because I read it as "the constrain that transforms the default constrain". But because it belongs to the transform, calling it transformConstrain is indeed not clear. I think "constrainOverride" is clearest now.

Let me know if I interpreted your comments correctly.

[HarelM]

Should the above defaultConstrain be part of the interface? I would imagine that this is something more internal...?

[larsmaxfield]

Yes, GlobeTransform relies on it being available when it calls it from this.currentTransform (ITransform):

    public constructor(options?: TransformOptions) {
        this._helper = new TransformHelper({
            calcMatrices: () => { this._calcMatrices(); },
            defaultConstrain: (center, zoom) => { return this.defaultConstrain(center, zoom); }

    ...

    defaultConstrain: TransformConstrainFunction = (lngLat, zoom) => {
        return this.currentTransform.defaultConstrain(lngLat, zoom);
    };

[HarelM]

Not the best design, but I guess it would do.

[HarelM]

Thanks! This looks good now.
I've added one last question before this can be merged.

[larsmaxfield]

Thanks again for your advice and review!