[website] Migrate 'plot' example to deck.gl v9 #8289

donmccurdy · 2023-11-15T21:24:24Z

Fixes [Bug] OrbitView example broken in v9 #8266

Updates examples/website/plot to deck.gl v9. The migration was easier done with type-checking, and as discussed in #8266 I've left the layer files in TypeScript. I'm also happy to revert the types if we prefer that examples be plain JavaScript.

There were a few things in the PR I wasn't sure about, and I'll add comments inline to call those out.

donmccurdy · 2023-11-15T21:33:09Z

examples/website/plot/plot-layer/axes-layer.ts

        vertexCount: gridPositions.length / 3
-      }),
-      isInstanced: true
+      })


This works, and types say isInstanced no longer exists, but is some indication of instancing expected at this point? state.instanceCount is not readily available but could be derived if necessary.

@ibgreen is this safe to remove? I see we still have it in ScatterplotLayer and other layers

donmccurdy · 2023-11-15T21:34:25Z

examples/website/plot/plot-layer/axes-layer.ts

      id: `${this.props.id}-grids`,
      vs: gridVertex,
      fs: fragmentShader,
+      bufferLayout: this.getAttributeManager().getBufferLayouts(),


Note: When bufferLayout is omitted, type checking still passes and the layer silently displays nothing. To make updates to v9 a bit easier, I wonder if the types could be more strict about this?

luma will autocreate the buffer layout for any attributes passed (so here just what is defined in the geometry, so I can see the logic in bufferLayout being optional. However, this pattern of adding more attributes via bufferLayout seems to be very common (at least in deck) and so maybe it would be good to make the type required, so that the user has to explicitly pass in [] to signal no additional buffers. @ibgreen thoughts?

In terms of failing silently, luma does complain if it cannot match a buffer, e.g.:

Model(PlotLayer-surface-surface): Ignoring buffer "instancePickingColors" for unknown attribute "instancePickingColors"

examples/website/plot/plot-layer/axes-layer.ts

donmccurdy · 2023-11-15T21:37:43Z

examples/website/plot/plot-layer/surface-layer.ts

      return -1;
    }
-    return [r / 255, g / 255];
+    return [r / 255, g / 255] as $TODO;


Probably not a huge deal, but TypeScript complains that these types differ from the base class.

I would remove this method entirely, and rely on the Layer implementation. Then, modify getPickingInfo to reconstruct [r, g, b] from the index

examples/website/plot/plot-layer/surface-layer.ts

examples/website/plot/tsconfig.json

examples/website/plot/plot-layer/axes-layer.ts

examples/website/plot/plot-layer/surface-layer.ts

felixpalmer · 2023-11-16T11:51:28Z

examples/website/plot/plot-layer/surface-layer.ts

      return -1;
    }
-    return [r / 255, g / 255];
+    return [r / 255, g / 255] as $TODO;


I would remove this method entirely, and rely on the Layer implementation. Then, modify getPickingInfo to reconstruct [r, g, b] from the index

examples/website/plot/tsconfig.json

felixpalmer · 2023-11-16T11:55:47Z

examples/website/plot/plot-layer/axes-layer.ts

        vertexCount: gridPositions.length / 3
-      }),
-      isInstanced: true
+      })


@ibgreen is this safe to remove? I see we still have it in ScatterplotLayer and other layers

examples/website/plot/plot-layer/axes-layer.ts

donmccurdy · 2023-11-16T17:01:50Z

Moving some inline conversations into main thread, as they've gotten detached in a rebase.

Try and resolve the remaining issues so we don't need this. I assume you're running the example using yarn start in the example directory.

I've added additional type checks so that strict: false is no longer necessary.

As far as I can tell we do need a tsconfig.json file — without it TypeScript raises many errors that seem to indicate it's type-checking against the wrong version, or multiple versions, of deck.gl and its dependencies.

Also test running the website (cd deck.gl/website && yarn && yarn start). It is currently failing.

Fixed by adding allowDeclareFields to deck.gl/website/babel.config.js, so that we can extend the base type declaration of Layer#state.

I would remove this method entirely, and rely on the Layer implementation...

I agree this would be better, but the direct conversion is not working as I would've expected. Perhaps I've missed something about why these methods were added. For now I've just fixed the types to match the base class so we don't need $TODO here, and left the overridden methods alone.

If there are other concerns with the types, perhaps I should remove TypeScript from the PR for now? It might be easier to deal with the TS issues in a PR that isn't also reworking the code for v9.

felixpalmer · 2023-11-20T10:12:48Z

I think it is best to not modify the typescript config with allowDeclareFields in this PR. If using declare fields is a good solution, we should adopt it in the whole codebase when dealing with state!. If you think this is better than what we have now, could you create an PR which migrates all the layers to using declare so others can comment? It might be better to first create an Issue to save time though. For background, I found this explanation useful.

I can't figure out why your layer definition of state leads to an error when ScatterplotLayer and others seem to be doing the same thing and there is no error though.

I think it would be a shame to remove the TypeScript as you've put in the effort and the types are helpful. In order to land this PR:

Remove the modifications to tsconfig
Where possible replace $TODO with the correct type, where not use any
Fix the issue with state!, using ts-ignore if you can't find a solution

donmccurdy · 2023-11-20T21:23:02Z

Remove the modifications to [babel config]

Where possible replace $TODO with the correct type, where not use any

Fix the issue with state!, using ts-ignore if you can't find a solution

✅ Done!

Unblocks extending layer state in TS.

donmccurdy commented Nov 15, 2023

View reviewed changes

donmccurdy marked this pull request as ready for review November 15, 2023 21:45

donmccurdy force-pushed the donmccurdy/example-plot-v9 branch from ad036c1 to 2c1b5e2 Compare November 15, 2023 21:47

donmccurdy mentioned this pull request Nov 15, 2023

[Bug] OrbitView example broken in v9 #8266

Closed

7 tasks

donmccurdy commented Nov 15, 2023

View reviewed changes

examples/website/plot/plot-layer/axes-layer.ts Outdated Show resolved Hide resolved

felixpalmer requested changes Nov 16, 2023

View reviewed changes

donmccurdy commented Nov 16, 2023

View reviewed changes

examples/website/plot/plot-layer/axes-layer.ts Outdated Show resolved Hide resolved

donmccurdy force-pushed the donmccurdy/example-plot-v9 branch from 2c1b5e2 to 5caed4e Compare November 16, 2023 16:34

donmccurdy force-pushed the donmccurdy/example-plot-v9 branch from c60e21e to 84b14c8 Compare November 16, 2023 21:46

donmccurdy force-pushed the donmccurdy/example-plot-v9 branch from 0167360 to c90722d Compare November 20, 2023 21:21

felixpalmer approved these changes Nov 21, 2023

View reviewed changes

felixpalmer mentioned this pull request Nov 21, 2023

Website v9 issue tracker #8193

Closed

23 tasks

Don McCurdy added 7 commits November 21, 2023 13:32

[website] Migrate 'plot' example to deck.gl v9

10c3cb3

Remove unneeded parameters

c22077c

Fix some type issues, remove strict:false

22f7c3d

Add 'allowDeclareFields' to website build

2e1cb9d

Unblocks extending layer state in TS.

Clean up TS

b584744

Clean up TS

8113dc5

Stricter types, remove remaining types

3cb2163

donmccurdy force-pushed the donmccurdy/example-plot-v9 branch from c90722d to 3cb2163 Compare November 21, 2023 19:32

Clean up

b898a9b

donmccurdy merged commit 2c81ee9 into master Nov 21, 2023

donmccurdy deleted the donmccurdy/example-plot-v9 branch November 21, 2023 19:46

donmccurdy mentioned this pull request Nov 21, 2023

[website] Clean up TS types in 'plot' example #8305

Merged

donmccurdy added this to the v9.0 milestone Jan 11, 2024

[website] Migrate 'plot' example to deck.gl v9 #8289

[website] Migrate 'plot' example to deck.gl v9 #8289

Uh oh!

Conversation

donmccurdy commented Nov 15, 2023

Uh oh!

donmccurdy Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

felixpalmer Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

donmccurdy Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

felixpalmer Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

donmccurdy Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixpalmer Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixpalmer Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felixpalmer Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

donmccurdy commented Nov 16, 2023

Uh oh!

felixpalmer commented Nov 20, 2023

Uh oh!

donmccurdy commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

donmccurdy Nov 15, 2023 •

edited

Loading

donmccurdy commented Nov 20, 2023 •

edited

Loading