Type matrix and vector calculations

[mourner]

Currently, all of our matrix/vector calculations are essentially untyped. This is because we use a very old version of Flow that makes it easy to make the code untyped unintentionally — in this case, throughout the codebase, we use the following pattern:

import {vec3} from 'gl-matrix';
...
function fn(a: vec3, b: vec3): vec3 { ... }

vec3 as imported from gl-matrix library is a namespace containing methods like add, transformMat3 etc., but since it's not explicitly typed anywhere, old Flow considers this as just Object, which is equivalent to any, so any code that is annotated like this can accept anything and Flow will never show any errors.

We started this pattern at some point and never noticed the problem, but it's now spread across the whole codebase — with vec2, vec3, vec4, mat2, mat3, mat4 and quat.

The new stricter types will allow us to both avoid errors (e.g. losing precision by expecting Float64Array and unintentionally creating Float32Array instead), and will be a step towards being able to upgrade to the latest Flow version, because it's much stricter and no longer allows these kinds of typing mistakes.

• Type all matrix/vector types and methods used throughout the code
• Fix all type references (vec2 -> import type {Vec2} from 'gl-matrix')
• Fix errors from type mismatches (e.g. typed as Float32Array but passed Array).

[mourner]

Note: I didn't use tuples for vec/matrix types (e.g. [number, number]) because we can't enforce arity on typed arrays like Float32Array, and also because it's a common pattern to start with [] when creating a matrix, where it grows to the necessary arity dynamically.

This would mean we could use the same type for all of these types, but I chose to use different names for the same thing (Vec2, Mat2 which are all array-like) because it helps self-documenting the code and showing intent.

[mourner]

Most of the remaining errors are about incompatibilities between various ways to represent a matrix (in different places we expect/use Float64Array, Float32Array and Array inconsistently). It might be a good time to revisit the topic of settling on one way to deal with matrices — e.g. maybe switch to array-only so that we always have double precision — maybe needs benchmarking for how it affects performance. cc @ansis

[mourner]

Ah, looks like this needs resolving conflicts after #11395.

[mpulkki-mapbox]

I believe reducing precision of some of the intermediate matrices from 64 bits to 32 bits will become problematic. Globe matrix is pretty much equivalent of tile matrices (createTileMatrix for other projections) meaning that it requires high precision for representing pixel coordinates at all zoom levels. Any precision issues might be masked though by the low zoom threshold value we have for the globe view but it will eventually bite us back.

[mourner]

Yeah, that's the moment I want to check up on — which of these matrices are intermediary and need high precision? I tried to keep them Float64Array but had a hard time reconciling typing errors when some of these matrices eventually get passed to WebGL methods that expect Float32Array.

[mourner]

@mpulkki-mapbox I reverted globe matrix to double precision — any other matrices I need to do this for?

[mpulkki-mapbox]

The rule of thumb would be that any matrix that is multiplied with transform.projMatrix should be kept 64bit. I'll go through the list quickly.

[mpulkki-mapbox]

@mourner: it's actually quite difficult to tell whether current type annotations for matrices are correct or not. For example the function signature calculateProjMatrix(unwrappedTileID: UnwrappedTileID, aligned: boolean = false): Float32Array in transform.js actually returns 64bit matrices as all possible code paths inside the function returns Float64Arrays. Similar patterns exists in other parts too. Or am I overlooking something perhaps?

[mourner]

@mpulkki-mapbox how so? It's explicitly returning Float32Array at the moment:

mapbox-gl-js/src/geo/transform.js

Lines 1444 to 1445 in 74559af

	cache[projMatrixKey] = new Float32Array(posMatrix);
	return cache[projMatrixKey];

The new type mismatches in the new branch are caught pretty well — certainly a huge improvement over silently passing mistypes in main.

[mpulkki-mapbox]

oh you're correct! The conversion is done at the end of the function call :). It's perhaps the calculatePosMatrix I was looking at. That function will forward the matrix creation to a specific TileTransform class and both FlatTileTransform and GlobeTileTransform will return 64 bit matrices. Am I looking things correctly now? :)

[mourner]

@mpulkki-mapbox I reverted createTileMatrix to Float64Array, so now it should at least be consistent. I guess it can't fully figure out the indirection in this instance to see that it doesn't match the type defined in projection/index.js, but upgrading Flow to the latest (2.5 years newer) version with much more sound typing architecture (which is my end goal) might help.

[karimnaaji]

The change looks good, I have a couple more questions to make sure we don't introduce functional changes with regards to Float32 vs Float64.

[karimnaaji]

👍