docs: Viewport elevation control #9313

charlieforward9 · 2024-12-20T18:20:26Z

Closes #9273
Linked to visgl/loaders.gl#3178

Background

I could not figure out how to control the view state when rendering Tile3dLayers in Montana, well above sea level. When I filed a report, I was told:

Does position: [0, 0, elevation]; work for you?

The MapViewState.position documentation did not specify elevation, leading me to overlooking it and ultimately filing this bug.

Adding some docs and using a stricter type would enhance DX.

Change List

MapViewState.position: number[] to [number, number, number]
... TODO

charlieforward9 · 2024-12-20T18:23:02Z

With this out of the way, can someone please help me figure out why the triplayer doesnt update properly #9260

modules/core/src/views/map-view.ts

charlieforward9 · 2025-01-07T21:10:37Z

@felixpalmer I see you rebased but may have forgotten to rerun the checks?

felixpalmer · 2025-01-08T07:17:44Z

The "new merge experience" helpfully hides that button 🙄

felixpalmer · 2025-01-08T07:18:19Z

@charlieforward9 can you run yarn test browser locally?

charlieforward9 · 2025-01-08T15:01:39Z

@felixpalmer I can, I get strange results though.

A bunch of Errors occur to unrelated modules in the repo, and it stops running at jupyter-widget Render Test Jupyter#ScatterplotLayer, logging: Failed to load url /modules/jupyter-widget/dist/index.js (resolved id: /modules/jupyter-widget/dist/index.js). Does the file exist?. When I try to build the jupyter widgets module, I get more problems.

I am able to find the remaining build errors much easier by running yarn run build

felixpalmer · 2025-01-08T16:31:31Z

Yes, the local test runs can sometimes throw up spurious errors. In general, if the CI isn't failing, but locally it is, then as a first step you can disable the test by commenting it out. Obviously if you can find the root cause, even better - but don't let it derail this PR

charlieforward9 · 2025-01-08T16:46:33Z

modules/geo-layers/src/tile-3d-layer/tile-3d-layer.ts

@@ -241,7 +241,7 @@ export default class Tile3DLayer<DataT = any, ExtraPropsT extends {} = {}> exten
      return;
    }

-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
+    //@ts-expect-error Type 'Vector3' is not assignable to type '[number, number, number]'


Got this error and didnt want to go down the rabbit hole of enforcing only [number, number, number]. What do you advise? @felixpalmer

Furthermore, there was a type mismatch error between the viewport defined here and a viewport type defined in @loaders.gl, since the project/unproject method signatures are changing from number[] to [number, number, number]. Seems like a PR needs to be made in the @loaders.gl repo as well.

I believe to have broken something cause now the test hangs on the Tile3DLayer initial load step and I do not get an error message... meanwhile, yarn run build completes successfully.

Opened visgl/loaders.gl#3178 to address this.

chrisgervang

Thank you for your continued work around auditing these types. It looks like the proposal has expanded a bit to include some API changes, which could mean waiting to release this PR in the next major version just so that we're not surprising users with a breaking change in a minor release.

For a patch or minor release, you could consider how to split out the doc and type changes into their our PR.

Thanks for understanding. Any sub-set of these improvements will be widely appreciated.

docs/upgrade-guide.md

chrisgervang · 2025-01-08T17:30:59Z

modules/core/src/lib/layer.ts

-    return xyz.length === 2 ? [x, y] : [x, y, z];
+    return [x, y, z];


I consider this a breaking API change. Could xyz be typed as [number, number] | [number, number, number]?

Not saying yet that we can't make this API change, but generally we'll tradeoff type safety to preserve the API, and not the other way around.

Even that will be breaking change as many people out there are likely passing a number[] and will need to add a cast. I would suggest using a function overload, something like:

function project(xyz: [number, number]): [number, number]; function project(xyz: [number, number, number]): [number, number, number]; function project(xyz: Vector2Or3): Vector2Or3 {

Also we haven't got around to using them everywhere yet, but we have helper types like NumberArray2 that you could use here

modules/core/src/viewports/globe-viewport.ts

charlieforward9 · 2025-01-08T19:07:19Z

@chrisgervang I adjusted most of the review comments. I will not be able to commit to dividing this PR. However, I'd love to become a contributor to this repo, and am patient to wait to see this land till the next major release. In the meantime, I am more than willing to fortify the current changes to be as clear as possible for when it does land.

Thank you for the review!

felixpalmer · 2025-01-09T08:27:28Z

modules/core/src/lib/layer.ts

-    return xyz.length === 2 ? [x, y] : [x, y, z];
+    return [x, y, z];


Even that will be breaking change as many people out there are likely passing a number[] and will need to add a cast. I would suggest using a function overload, something like:

function project(xyz: [number, number]): [number, number]; function project(xyz: [number, number, number]): [number, number, number]; function project(xyz: Vector2Or3): Vector2Or3 {

felixpalmer · 2025-01-09T08:29:15Z

modules/aggregation-layers/src/contour-layer/contour-layer.ts

@@ -143,7 +143,7 @@ export default class GridLayer<DataT = any, ExtraPropsT extends {} = {}> extends
          getValue: ({positions}: {positions: number[]}, index: number, opts: BinOptions) => {
            const viewport = this.state.aggregatorViewport;
            // project to common space
-            const p = viewport.projectPosition(positions);
+            const p = viewport.projectPosition([positions[0], positions[1], positions[2] || 0]);


The original motivation for this PR is to improve the types, so I would avoid changing the implementation. Adding type casts or better types is great, but you should avoid anything that will modify the behavior at runtime as there is a risk of introducing bugs

felixpalmer · 2025-01-09T08:32:02Z

modules/aggregation-layers/src/contour-layer/contour-layer.ts

@@ -272,7 +275,7 @@ export default class GridLayer<DataT = any, ExtraPropsT extends {} = {}> extends
      binIdRange = getBinIdRange({
        dataBounds: bounds,
        getBinId: (p: number[]) => {
-          const positionCommon = viewport.projectFlat(p);
+          const positionCommon = viewport.projectFlat([p[0], p[1]]);


Same comment as above, better to do p as NumberArray2

felixpalmer · 2025-01-09T08:33:32Z

modules/core/src/lib/layer.ts

-    return xyz.length === 2 ? [x, y] : [x, y, z];
+    return [x, y, z];


Also we haven't got around to using them everywhere yet, but we have helper types like NumberArray2 that you could use here

docs: elevation control in MapViewState

9f8e01f

ibgreen approved these changes Dec 21, 2024

View reviewed changes

modules/core/src/views/map-view.ts Show resolved Hide resolved

charlieforward9 and others added 5 commits December 21, 2024 16:11

Add MapViewState change documentation

e6b304d

Merge branch 'master' into patch-1

e20e91c

fix: strictly include elevation in position type and docs

33dde58

docs: ViewportOptions position type change

9d8647a

Merge branch 'master' into patch-1

e306dfe

charlieforward9 commented Jan 8, 2025

View reviewed changes

chrisgervang reviewed Jan 8, 2025

View reviewed changes

fix: enforce 3D coordinates across dependent modules

5c14463

charlieforward9 force-pushed the patch-1 branch from eb96c86 to 5c14463 Compare January 8, 2025 18:58

fix: coordinate ordering

a8117e5

charlieforward9 changed the title ~~docs: elevation control in MapViewState~~ docs: Viewport elevation control Jan 8, 2025

charlieforward9 added a commit to NEW-HEAT/loaders.gl that referenced this pull request Jan 8, 2025

fix: update Viewport type for visgl/deck.gl#9313

9dbeb9e

charlieforward9 mentioned this pull request Jan 8, 2025

fix: update Viewport type visgl/loaders.gl#3178

Open

felixpalmer requested changes Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Viewport elevation control #9313

docs: Viewport elevation control #9313

charlieforward9 commented Dec 20, 2024 •

edited

Loading

charlieforward9 commented Dec 20, 2024

charlieforward9 commented Jan 7, 2025

felixpalmer commented Jan 8, 2025

felixpalmer commented Jan 8, 2025

charlieforward9 commented Jan 8, 2025 •

edited

Loading

felixpalmer commented Jan 8, 2025

charlieforward9 Jan 8, 2025

charlieforward9 Jan 8, 2025

charlieforward9 Jan 8, 2025 •

edited

Loading

charlieforward9 Jan 8, 2025

chrisgervang left a comment

chrisgervang Jan 8, 2025

felixpalmer Jan 9, 2025

felixpalmer Jan 9, 2025

charlieforward9 commented Jan 8, 2025

felixpalmer Jan 9, 2025

felixpalmer Jan 9, 2025

felixpalmer Jan 9, 2025

felixpalmer Jan 9, 2025

		return xyz.length === 2 ? [x, y] : [x, y, z];
		return [x, y, z];

docs: Viewport elevation control #9313

Are you sure you want to change the base?

docs: Viewport elevation control #9313

Conversation

charlieforward9 commented Dec 20, 2024 • edited Loading

Background

Change List

charlieforward9 commented Dec 20, 2024

charlieforward9 commented Jan 7, 2025

felixpalmer commented Jan 8, 2025

felixpalmer commented Jan 8, 2025

charlieforward9 commented Jan 8, 2025 • edited Loading

felixpalmer commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlieforward9 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisgervang 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

charlieforward9 commented Jan 8, 2025

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

charlieforward9 commented Dec 20, 2024 •

edited

Loading

charlieforward9 commented Jan 8, 2025 •

edited

Loading

charlieforward9 Jan 8, 2025 •

edited

Loading