Typescript: updates to gl-matrix (Draft - hard to test as tsc failing) #219

chippieTV · 2021-07-22T16:59:05Z

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

Some types seem to be Float32Array when it seems they should often be gl-matrix types and constructors.
Because we are in the middle of porting to Typescript it is hard to confirm that this doesn't break more than it fixes, however for my fork at the time of the MR the errors in tsx went from 615 down to 593.

UPDATE: latest 226 -> 202 // this is a moving target, latest is about 20 errors fixed by this branch (I assume 4 got merged in as unrelated issues)
UPDATE: last check I see 39 errors left after the changes in this branch

The PR is WIP, please comment and I'll do my best to fix.

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
apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>

HarelM

I think this is good, but I have a feeling some of the changes here are a bit risky currently.
I would say that I want everything working in terms of typescript and maplibre before introducing this change in order to know that this is not breaking anything.
Sorry if this is not what you have expected to read... :-(

src/geo/transform.ts

src/render/uniform_binding.ts

src/style/style_layer.ts

HarelM · 2021-07-23T05:24:56Z

src/style/style_layer/circle_style_layer.ts

@@ -69,7 +69,7 @@ class CircleStyleLayer extends StyleLayer {
                const transformedPoint = alignWithMap ? point : projectPoint(point, pixelPosMatrix);

                let adjustedSize = transformedSize;
-                const projectedCenter = vec4.transformMat4([], [point.x, point.y, 0, 1], pixelPosMatrix);
+                const projectedCenter = vec4.transformMat4(vec4.create(), [point.x, point.y, 0, 1], pixelPosMatrix);


We need to understand if this is a performance penalty here since we are not longer using []... I don't have the answers though...

I guess first question is how often it runs and is the penalty if there is one, something to be concerned about. I know when we spoke yesterday I voiced this exact concern about it.

Skimming over the code in gl-matrix, it looks like it will use a Float32Array if the browser has it, and if it does not it will fall back to standard Array (i.e. prefers it). create() will assign the initial values to 0 if it's not Float32Array for 12 of the 16 elements of a mat4 as new Array(16) returns undefined for all elements in the array, while the TypedArray is packed/aligned whichever.

It leads me to believe that actually it should be faster to create the TypedArray than an empty one which because it's of no fixed length, may need to be grown or can't be expected to know how long it should be or what is going into it.

That said, it seems of course it's more complex than that, and maybe the JS engine does something clever.. this post about Array vs Typed arrays is super interesting and I think I will read through it again a bit more carefully later

HarelM · 2021-07-23T05:25:15Z

src/style/style_layer/circle_style_layer.ts

-function projectPoint(p: Point, pixelPosMatrix: Float32Array) {
-    const point = vec4.transformMat4([], [p.x, p.y, 0, 1], pixelPosMatrix);
+function projectPoint(p: Point, pixelPosMatrix: mat4) {
+    const point = vec4.transformMat4(vec4.create(), [p.x, p.y, 0, 1], pixelPosMatrix);


Same here regarding performance.

for performance, same as above.

also updated second arg to transformMat4() to vec4.fromValues()

src/style/style_layer/fill_extrusion_style_layer.ts

src/symbol/collision_index.ts

chippieTV · 2021-07-23T09:06:49Z

I think this is good, but I have a feeling some of the changes here are a bit risky currently.
I would say that I want everything working in terms of typescript and maplibre before introducing this change in order to know that this is not breaking anything.
Sorry if this is not what you have expected to read... :-(

no problem. I'm more than happy for this stuff to sit and wait for there to be a way to test it properly, or merge and squash so we can revert at some point in the future if it turns out to be bad.

Is there any way we should title these PRs to keep them all together? like TYPESCRIPT: issue description... or tag perhaps? maybe there already is

HarelM · 2021-07-23T09:10:46Z

Typescript prefix sounds good.

src/render/uniform_binding.ts

src/geo/transform.ts

HarelM · 2021-07-26T08:23:29Z

src/render/draw_symbol.ts

@@ -6,7 +6,7 @@ import pixelsToTileUnits from '../source/pixels_to_tile_units';
 import * as symbolProjection from '../symbol/projection';
 import * as symbolSize from '../symbol/symbol_size';
 import {mat4} from 'gl-matrix';
-const identityMat4 = mat4.identity(new Float32Array(16));
+const identityMat4 = mat4.create();


Can we use identity just for the sake of readablility?

'identity()' resets an existing mat4, 'create()' creates a new one with identity

#219 (comment)

it's maybe minor but create() should be faster too, well at least it's running a few less operations.. it appears to me the following:

new Float32Array(16) will init a typed array of 16 fixed size elements each initialized to 0

identity() will set all 16 elements of an array (typed or not) no matter what they contain

[] will create a normal JS Array which is more flexible and slower as the elements are not aligned to the same set size (my understanding) allowing Array to hold any mixed kinds of elements.

create() checks if it can create a Float32Array (browser support) and if so will jump to just initializing the Float32Array(16) and since it knows it will be all zeros, it only sets 4 1's on the diagonal

Ahh, then never mind... Maybe a comment then?

I also moved it out of the imports which was a weird place for it to be honest

HarelM · 2021-07-26T08:59:41Z

src/symbol/collision_index.ts

-    getViewportMatrix(): mat4 {
-        const m = mat4.identity([]);
+    getViewportMatrix() {
+        const m = mat4.create();


Same as above about readability.

src/symbol/projection.ts

HarelM · 2021-07-26T10:35:52Z

Ok, I reviewed all the code. I had minor corrections. Let's hope this won't create any performance issues...
Let's continue forward with merging this...

updating types as gl-matrix more types to gl-matrix cleanup cast vs constructor cleanup more gl-matrix types more using gl-matrix instead of Float32Array fixes from WIP squashed into one commit minor cleanup and comments

chippieTV · 2021-07-26T10:55:58Z

sorry I force pushed because I want it to be easy to revert if it's wrong so it's a single commit. it might depend on the git workflow whether that's actually helpful or not.. what's better for you? I should check first

the diff is https://github.com/maplibre/maplibre-gl-js/compare/9902e2831214d7f9e57b7926056baf5bdd4b7475..3cb45dcd1f1fb1d2b25100c3afd445aec5fbfe7c

(hidden in the 'force-pushed' link) also I'm more used to gitlab so maybe what lives at these links Is obvious, took me a minute to figure out

HarelM · 2021-07-26T11:27:19Z

I'm squashing the commit, so it doesn't really matter how the commit in the branch are... I think...

* Update README.md * Initial commit - only rename and flow to typescript conversion * Iint upgrade * More lint fixes * Updated lint command, added build typescript command * Add non typed modules according to ts fork. * Switch from yarn to npm * Fix hasOwnProperty lint errors See https://eslint.org/docs/rules/no-prototype-builtins * I used this command for the conversions: * sed -i -E "s/([\(\!])([]/[a-zA-Z_.-]*).hasOwnProperty\(/ \1Object.prototype.hasOwnProperty.call(\2, /g" <file-name> * Fix extension from .js to .ts * Remove types file * Ignore lint errors * Remove .ts extension in import * Update CI from node v10 to v14 * Replace yarn with npm * Remove circleci * Fixed some typings errors * Explicitly convert matrix-gl types * Automatically fix lint errors * Avoid Function as callback type * Add missing MapOptions * Add pitchWithRotate to MapOptions and pass explicit options to HandlerManager and Camera * Set type of MapOptions.style to StyleSpecification of string * Make Camera an abstract class * Use Float32Array instead of Float64Array and cast types * Make object dynamically extendable See https://stackoverflow.com/questions/12710905/how-do-i-dynamically-assign-properties-to-an-object-in-typescript * Make more objects dynamically extenable * Explicit cast of array to length 2 * Do not provide type any in for..in loop * Make more objects dynamically extenable * Cast layout.get return value to Number * Cast number | void to number * Specify Rect type * Make thhird argument of getKey optional * Cast number | void to number * Define return type of parse() in subclasses of Expression * Cast return type of context.error() from void to null * Cast style-spec types to Type * Cast return type of context.error() from void to null * Cast style-spec types to Type * Cast dict to StylePropertySpecification * Specify array type as any to fix string[] inference * Make third argument of coalesce() optional * Explicit casts to Evaluate, Varargs, and Signature * Revert "Cast style-spec types to Type" This reverts commit 4af74a6. * Revert "Cast style-spec types to Type" This reverts commit efc4b1f. * Cast style-spec types at their source * Typescript: Remove @mapbox/geojson-types and replace with @types/GeoJSON' (#221) * remove @mapbox/geojson-types and replace with @types/GeoJSON * update toJSON using spread operator * add type to import GLSL as strings (#222) * Typescript: Use library to handle Window type (#223) * add window type lib * remove internal window type file and use lib * Fix issues related to possibly evaluated design * Move IControl interface to a new class, Fix timeoutID type. * remove flow-typed folder (#225) * Typescript: Remove reference to @mapbox/point-geometry TBC (#226) * Remove reference to @mapbox/point-geometry and replace with local point module with types Remove commented out types Fix cannot find type Point errors Add static to fix convert does not exist on typeof Point errors * Remove @mapbox/point-geometry from packkage.json * Fix lint * Typescript: fix lint and update .eslintrc with alternate TS rule (#227) * fix lint and .eslintrc with alternate TS rule * remove comment * Typescript: a number of minor fixes from WIP branch (#228) * fix minor ID type issues * minor casting fix non bool to bool * fix thrown Error message issue * fix a couple of import issues * match date comparison types * remove commented out code * fixes from WIP branch * remove redundant import for lint * more fixes * Fix small typings issues * More typescript fixes * More fixes to typings, lint fixes * Change base calls to be abstract * More type fixes * More types fixes * More types fixes * update use of mat4 (#219) updating types as gl-matrix more types to gl-matrix cleanup cast vs constructor cleanup more gl-matrix types more using gl-matrix instead of Float32Array fixes from WIP squashed into one commit minor cleanup and comments * Added some "as any" to places in the code where the typing were not just right and the method are proplemeatic... * More types fixing, not perfect but good enough. * Fix last typescript errors * Fix incorrect typings and code errors. * Remove flow from rollup and add tsc to build * Format file * Fix rollup build * Initial commit to make the render test run * Remove unneeded window import from all files * Remove version from API * More mock fixes to be able to run tests * fix running of test-expression (#232) * Add build phase before running some of the tests * Fix tests get stuck, still tests are failing... * get node query tests running (#233) * Fix mocking of case of tilejson implementation. * Typescript: Fix unit-tests (#235) * Move all referneces in unit test to use the build output * Fixes to window related stuff in unit tests, fixes to unneeded requires. * Fix to dem data test * Fix tests - remove window restore, fix __dirname. * Fix stub_loader due to function this usage. * Fix more tests * Fix tests and code that causes tests to fail. * Fix usage of ajax.js by subbing it correctly * Added missing window element to tests * removing window in 2 places gains 1601 passing (#236) * Adding missing windows to tests * Fix tests, revert some changes from typescript migration * More fixes related to removal of window * remove warnings, fix attribution test * Fix more tests * Fix more tests * Added missing stubs * Fixed tests related to version removal * More test fixes related to file path * Typescript: unit tests Frustum issues (#238) * fix minor file errors in stub_loader.js * matrix tests pass but not sure the tests represent actual usage * Fix primitive tests with minimal usage of "bad" types * Minor fixes to reduce incorrect typings * Fix image load for image source and style tests * Fix tests, added missing stub_loader * Fix uuid incorrect implemetation * Fix ajax tests. imporved image mock * Fix camera tests * Revert changes related to incorrect typescript solution Co-authored-by: James Hamilton <chippieTV@users.noreply.github.com> * Incorrect invocation of build-dev... * Fix lint errors * Removed test flow * Fix build csp, started to fix style build * Fix css build * Remove to vec3 as to code review requirements * Remove flow from build in ci * Remove references to @mapbox/point-geometry * Allow building style-spec * Fix build tests * Remove window, fix missing references to FeatureIndex using typescript config, update rollup dependencies * Fix lint * Fix production build * Remove buble, stop compiling for es5 * Added missing global definitions after removing window * Fix tests, bring back web_worker mock file, fix maplibre-gl loading issue, revert tsconfig. * Fix build? * Fix lint * Typescript: Fix query-tests (#240) * Initial commit to fix query-tests * Fix query tests * Fix some lint issues * Typescript: minor fixes to formatting (#241) * minor fixes * fix formatting of object key value types * Remove unneeded comments * Fix according to code review * Remove undefined as any * Fix according to code review requirements. * Fix lint * More lint cover and fixes * Fix lint for windows, update versions. * Fix browser tests due to incorrect lint fix * Fix lint for land.html * Fix minimal node version to 14 * Typescript: merge from main (#243) * Highlight backport rules * Add backport rules * Add backport rules * Use organization secret NPM_ORG_TOKEN (#231) * Fix link in README and CONTRIBUTING. Fix typo in README. (#237) * one yarn forgotten (#242) Co-authored-by: Marcel Normann <marcel.normann@wheregroup.com> Co-authored-by: Oliver Wipfli <oliver.wipfli@leichteralsluft.ch> Co-authored-by: tunnelpuzzle <79618312+tunnelpuzzle@users.noreply.github.com> * Update Life of a tile docs (#247) * rename .js to .ts in doc/life-of-a-tile.md * Update Developer Doc - Life of a tile (#245) * life of a tile * correction * Update package.json (#248) * rename .js to .ts in doc/life-of-a-tile.md * life of a tile * correction * package.json * some updates from review (#252) * Fix WritingMode cast (#254) * Run CI on all pull requests (#255) * Remove redundant parentheses (#257) * Remove (minify = false) * Review cleanup (#259) * Remove redundant parentheses * Remove eslint typescript exceptions * Remove flow comments * Remove `Number()` casts * Remove parentheses * Remove commented lint plugin * Remove `HM TODO` comments * Remove TODO, Fix according to code review changes * Remove last TODOs. * Updated changelog * Change publish style spec to reflect latest changes in typescript * Fix code review comments * Declare `emplace()` with variable number of arguments (#262) * Fix comment of generated files * Fix according to code review * remove comment and update type in jsdoc (#269) * Copy dist folder with `cp -r` (#274) * Fix typo * Fix last comments about generated files. * Fix typos in comments (#283) * Remove polyfill support for IE11 (#284) * Use native endsWith function * Use native Object.values function * Use native Number.MAX_SAFE_INTEGER value * Fix typos, remove unused functions The functions are already defined in `src/util/mapbox.ts` * Enforce semicolons as member delimters (#282) * eslint - enforce single quotes (#285) Turn on @typescript-eslint/quotes rule to standardize on single quotes for all strings. * move point.ts from symbol/ to util/ (#287) * move point.ts from symbol/ to util/ * Remove empty line * Remove undefined as any casts (#292) Unnecessary casts for 'undefined' or 'null' as 'any'. * Remove feature detection for {passive: false} support (#291) * Changed version in package.json before merge * Remove the usage of devicePixelRatio as a getter from browser.ts (#294) Co-authored-by: Oliver Wipfli <oliver.wipfli@leichteralsluft.ch> Co-authored-by: James Hamilton <chippieTV@users.noreply.github.com> Co-authored-by: Astrid <astridx@users.noreply.github.com> Co-authored-by: Marcel Normann <marcel.normann@wheregroup.com> Co-authored-by: tunnelpuzzle <79618312+tunnelpuzzle@users.noreply.github.com> Co-authored-by: Frédéric Junod <frederic.junod@camptocamp.com> Co-authored-by: Derek Westcott <drwestco@gmail.com>

HarelM requested changes Jul 23, 2021

View reviewed changes

chippieTV force-pushed the typescript-gl-matrix branch from f1deca2 to ebc2152 Compare July 23, 2021 09:01

chippieTV commented Jul 23, 2021

View reviewed changes

src/render/uniform_binding.ts Outdated Show resolved Hide resolved

chippieTV changed the title ~~WIP Typescript updates to gl-matrix~~ Typescript: updates to gl-matrix (WIP) Jul 23, 2021

chippieTV changed the title ~~Typescript: updates to gl-matrix (WIP)~~ Typescript: updates to gl-matrix (Draft - hard to test as tsc failing) Jul 23, 2021

chippieTV force-pushed the typescript-gl-matrix branch 6 times, most recently from 527d2b7 to 816625d Compare July 26, 2021 04:23

chippieTV mentioned this pull request Jul 26, 2021

Typescript: TMP WIP further fixes including window and point.ts (rebase on typescript gl matrix) #224

Closed

4 tasks

chippieTV force-pushed the typescript-gl-matrix branch from 816625d to 9902e28 Compare July 26, 2021 08:18

HarelM reviewed Jul 26, 2021

View reviewed changes

src/geo/transform.ts Show resolved Hide resolved

HarelM reviewed Jul 26, 2021

View reviewed changes

src/symbol/projection.ts Show resolved Hide resolved

update use of mat4

3cb45dc

updating types as gl-matrix more types to gl-matrix cleanup cast vs constructor cleanup more gl-matrix types more using gl-matrix instead of Float32Array fixes from WIP squashed into one commit minor cleanup and comments

chippieTV force-pushed the typescript-gl-matrix branch from 9902e28 to 3cb45dc Compare July 26, 2021 10:55

HarelM approved these changes Jul 26, 2021

View reviewed changes

HarelM merged commit c48e970 into maplibre:typescript Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescript: updates to gl-matrix (Draft - hard to test as tsc failing) #219

Typescript: updates to gl-matrix (Draft - hard to test as tsc failing) #219

chippieTV commented Jul 22, 2021 •

edited

Loading

HarelM left a comment

HarelM Jul 23, 2021

chippieTV Jul 23, 2021

HarelM Jul 23, 2021

chippieTV Jul 23, 2021

chippieTV commented Jul 23, 2021

HarelM commented Jul 23, 2021

HarelM Jul 26, 2021 •

edited

Loading

chippieTV Jul 26, 2021

chippieTV Jul 26, 2021

HarelM Jul 26, 2021

chippieTV Jul 26, 2021

HarelM Jul 26, 2021

HarelM commented Jul 26, 2021

chippieTV commented Jul 26, 2021 •

edited

Loading

HarelM commented Jul 26, 2021

Typescript: updates to gl-matrix (Draft - hard to test as tsc failing) #219

Typescript: updates to gl-matrix (Draft - hard to test as tsc failing) #219

Conversation

chippieTV commented Jul 22, 2021 • edited Loading

HarelM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chippieTV commented Jul 23, 2021

HarelM commented Jul 23, 2021

HarelM Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarelM commented Jul 26, 2021

chippieTV commented Jul 26, 2021 • edited Loading

HarelM commented Jul 26, 2021

chippieTV commented Jul 22, 2021 •

edited

Loading

HarelM Jul 26, 2021 •

edited

Loading

chippieTV commented Jul 26, 2021 •

edited

Loading