Transform cleanup

[brunoabinader]

A follow-up to #13445, this PR refactors Transform to reduce its API surface.

[brunoabinader]

😍

[pozdnyakov]

Looks good overall

[pozdnyakov]

that's out of this patch scope, however, is it fine we just return here?

[brunoabinader]

We could transform this into an assertion, or throw an exception (though I know you're not fond of it) ;) but I'd prefer to treat this in a separate issue/PR.

[brunoabinader]

@pozdnyakov applied review changes 👍 thanks! ✔️ ?

[pozdnyakov]

LGTM

[julianrex]

What's this line doing?

[1ec5]

It’s still adjusting the frame of reference for the center coordinate, but now it does so by padding all four sides so that the padding covers the entire view, instead of setting an anchor point explicitly.

[brunoabinader]

Yes - as @1ec5 mentioned, this was previously done inside an overload for setLatLng in Transform that also received an anchor point as a parameter - this overload has been removed in this PR to avoid confusion.

[1ec5]

It’s still adjusting the frame of reference for the center coordinate, but now it does so by padding all four sides so that the padding covers the entire view, instead of setting an anchor point explicitly.

[1ec5]

In the past, we avoided designated initializer lists because certain compilers we targeted didn’t support them. Is that still the case, or could we use designated initializer lists instead of the builder pattern?

transform.jumpTo(CameraOptions {
    .center = LatLng { 0.1, -0.1 },
    .zoom = 8.0,
    .angle = 5.0,
    .pitch = 40.0,
});

Both syntaxes provide more clarity than unlabeled constructor parameters, but I find designated initializer lists to be more elegant. We already use designated initializer lists liberally in Objective-C++ code in platform/{darwin,ios,macos}/, since Clang supports them as an extension to C++. For example:

mapbox-gl-native/platform/darwin/src/MGLGeometry.mm

Lines 114 to 119 in 0addd51

	MGLMatrix4 mat4 = {
	.m00 = array[0], .m01 = array[1], .m02 = array[2], .m03 = array[3],
	.m10 = array[4], .m11 = array[5], .m12 = array[6], .m13 = array[7],
	.m20 = array[8], .m21 = array[9], .m22 = array[10], .m23 = array[11],
	.m30 = array[12], .m31 = array[13], .m32 = array[14], .m33 = array[15]
	};

[brunoabinader]

Though I'm quite fond of designated initializer lists too, this technique is a bit different as it links chained setters via a reference to the CameraOptions object, but the constructor still remains empty. In C++, support for designated initializers comes in C++20, so it'll take a while until all our supported platforms can get availability on this.

[1ec5]

For future reference, Clang has supported designated initializers in C++ for years as an extension to the C++ standard. GCC at some point introduced support for “trivial designated initializers”, which means the fields have to come in the same order as they’re declared, like in Swift.

/ref #26 #4722

[asheemmamoowala]

The order of results returned by tile cover is important. It is supposed to radiate out from the center. Is there a reason that the order and number of tiles is changing?

[brunoabinader]

@asheemmamoowala yes - previously, the bearing (angle) was set in the transform object, which receives bearing in radians. The value, however, is 5.0, which I assume it is supposed to indicate a value in degrees. The updated code fixes this typo and has the bearing value set via CameraOptions, which now properly receives degree values for both bearing and pitch.