Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(core) new device props api #2100

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

chrisgervang
Copy link
Contributor

@chrisgervang chrisgervang commented Jun 13, 2024

For visgl/deck.gl#8945

Background

DeviceProps API Changes:

For both devices,

  • Nest device specific props.
  • Allow setting more default canvas context props when creating a device.

For webgl,

  • remove onContextLost. Use device.lost instead.
  • remove onContextRestore. Luma 9 doesn't support restoring context.

For WebGPU,

  • Allow setting more default canvas context props when creating a WebGPU device.

Taking a stab at documenting device props per device and type hinting CreateDeviceProps. Feel free to make corrections or clarifications around the intent behind some of these props, when I was unsure I went off of how the current implementation is written (even though knowing it may be incomplete)

I think deck should use CreateDeviceProps, so I've exported that as well.

Change List

  • types
  • docs

Signed-off-by: Chris Gervang <chris@gervang.com>
Signed-off-by: Chris Gervang <chris@gervang.com>
};

export type WebGPUDeviceProps = _DeviceProps & {
/** Request a Device with the highest limits supported by platform. WebGPU: devices can be created with minimal limits. */
requestMaxLimits?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended to be WebGPU only? It isn't used by WebGL

modules/core/src/adapter/device.ts Outdated Show resolved Hide resolved
modules/core/src/adapter/luma.ts Show resolved Hide resolved
Comment on lines 297 to 305
alpha: true,
depth: true,
stencil: false,
desynchronized: false,
antialias: true,
failIfMajorPerformanceCaveat: false,
powerPreference: 'high-performance',
premultipliedAlpha: true,
preserveDrawingBuffer: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've based these defaults on what the docs say. I'm not sure what desynchronized should be by default since the doc was incomplete

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite convinced that this is worth the extra complexity in term of types etc, compared to just adding two WebGL/WebGPU columns to the DeviceProps docs table.

Also seems to limit the ability to specify WebGL/WebGPU options when using best-available (though I assume that is fixable).

@@ -128,11 +128,13 @@ export class WebGLDevice extends Device {

this.handle = createBrowserContext(this.canvasContext.canvas, {
...props,
onContextLost: (event: Event) =>
onContextLost: (event: Event) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these changes? Some editor auto correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calls the users onContextLost, if provided

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calls the users onContextLost, if provided

I see. Adding a one line bug fix in to a bigger API change is a bit risky, can slip through the review, and can get lost if API change is not approved...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to split this out. I didn't mean to put anything risky in here. The reason I included this is get clarity on the intended API. It's unclear to me what the intent is.

If users shouldn't be able to provide onContextLost by design, then the API types need to be adjusted a bit. If we can agree the user should be able to provide onContextLost, then this is a bug and I'll pull that into another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you shine a light on it, I think we should completely drop support for onContextLost. Such callback code tends to be surprisingly messy to keep working across devices and it doesn't add value.

We don't talk about contexts anymore, just devices. and we have the replacement in the WebGPU style promise API: device.lost

App can register its onContextLost callback with device.lost.then(onContextLost).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. device.lost sounds better since it the same API for both devices, making this redundant.

Is there a device equivalent for onContextRestore as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, context restore is not supported by luma.gl v9.

The problems is that properly handling context restore in non-trivial apps is very tricky, since all resources are lost and have to be recreated, and the data that was used to create buffers and textures may not have been saved.

\Showing a message asking the user to just refresh the page is usually best way out.

WebGPU doesn't support restore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the onContextRestore code in the WebGL device be removed then?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I would vote for that. It is not impossible for an app to register the callback even if luma.gl API doesn't support it, they will just need to get the context and add an event handler themselves.

@chrisgervang
Copy link
Contributor Author

The vision here makes sense to me, that one day we’ll be able to use best-available and luma will match the default behaviors between the devices as best as it can. However, I still believe there’s a need for discriminated union types.

The complex typing reflects a complex API; deviceProps are effectively two different objects (or the union of those) depending on their use case, and then another union with additional properties for createDevice. These APIs are challenging to explain, but types can help.

I think these types should be as helpful to users as possible. It defeats the purpose if users need to constantly refer to the documentation just to know which properties they can use, especially when they are explicitly using WebGL or WebGPU.

I think the goal should be to provide clarity and reduce the cognitive load, and I'm open simpler implementations if they exist.

@ibgreen
Copy link
Collaborator

ibgreen commented Jun 18, 2024

The vision here makes sense to me, that one day we’ll be able to use best-available and luma will match the default behaviors between the devices as best as it can. However, I still believe there’s a need for discriminated union types.

The complex typing reflects a complex API; deviceProps are effectively two different objects (or the union of those) depending on their use case, and then another union with additional properties for createDevice. These APIs are challenging to explain, but types can help.

I think these types should be as helpful to users as possible. It defeats the purpose if users need to constantly refer to the documentation just to know which properties they can use, especially when they are explicitly using WebGL or WebGPU.

I think the goal should be to provide clarity and reduce the cognitive load, and I'm open simpler implementations if they exist.

In loaders.gl we have options sub-objects for each loader, the same model could work here:

luma.createDevice({
 // common options
 webgl: {
   // webgl specific options,
 },
 webgpu: {
   // webgpu specific options
 }
};

It would be slightly breaking but make it very clear what is going on.

Signed-off-by: Chris Gervang <chris@gervang.com>
Signed-off-by: Chris Gervang <chris@gervang.com>
@chrisgervang chrisgervang changed the title chore(core) document device props per device chore(core) new device props Jun 20, 2024
@chrisgervang chrisgervang changed the title chore(core) new device props chore(core) new device props api Jun 20, 2024
Signed-off-by: Chris Gervang <chris@gervang.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants