Skip to content

Change GPUColor/Origin3D/Extent3D to sequences#299

Closed
kainino0x wants to merge 1 commit intogpuweb:masterfrom
kainino0x:vecs-as-sequences
Closed

Change GPUColor/Origin3D/Extent3D to sequences#299
kainino0x wants to merge 1 commit intogpuweb:masterfrom
kainino0x:vecs-as-sequences

Conversation

@kainino0x
Copy link
Contributor

This makes WebGPU app code less verbose/hard to type. Without this, I think apps are likely to have their own boilerplate helpers: makeGPUColor = (r, g, b, a) => ({r, g, b, a}), etc.

Open for debate.

Copy link
Contributor

@kvark kvark left a comment

Choose a reason for hiding this comment

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

I'd prefer not to go this way. Sequences involve indirection, and they have weaker type-level semantics than our structures:

  • no field names
  • no forced number of fields

@kainino0x
Copy link
Contributor Author

Even though it's not expressed by the IDL, I'd like to express those constraints in the spec.

@grovesNL
Copy link
Contributor

I'd prefer the current approach for the same reasons as @kvark.

I guess typed arrays could be another option instead of raw sequences. Typed arrays would probably increase verbosity, but would enforce type for values (i.e. new Uint32Array([r, g, b]);) and there's some precedent in WebGL (i.e. gl.getParameter(gl.VIEWPORT); -> Int32Array[0, 0, 640, 480])

Maybe we'll have typed objects someday :)

@kainino0x
Copy link
Contributor Author

FYI: Blocked on @jdashg's alternative proposal.

@kainino0x
Copy link
Contributor Author

Closed in favor of #319

@kainino0x kainino0x closed this Jun 17, 2019
@kainino0x kainino0x deleted the vecs-as-sequences branch April 27, 2020 23:59
@kainino0x
Copy link
Contributor Author

Recently I'm again finding myself wanting to only allow sequences, for simplicity of the spec and implementation, especially with the merging of arrayLayers into depth/z. Is there any appetite to revisit this? (@kvark in particular)

Specifically I would want to do this only for Origin and Extent (not color, as the order is not so obvious), and we would have extra bindings rules for sequence length.

@kainino0x
Copy link
Contributor Author

It eliminates the ambiguity for 1d-array textures: #613 (comment)

kainino0x added a commit to kainino0x/gpuweb that referenced this pull request Sep 16, 2020
This overload adds complexity without providing much benefit of clarity.
In all contexts, the semantics of the values in an array used as a
GPUOrigin/Extent are clear; depending on the context, one of:

- [width]
- [width, arraysize]
- [width, height]
- [width, height, arraysize]
- [width, height, depth]

In order to implement this, this redefines GPUOrigin2D, GPUOrigin3D, and
GPUExtent3D from scratch, as if they were specified in the WebIDL spec.
These essentially implement a specific case of tuples with default
values, which could be expressed in pseudo-WebIDL as:

```
typedef [GPUIntegerCoordinate = 0, GPUIntegerCoordinate = 0] GPUOrigin2D;
typedef [GPUIntegerCoordinate = 0, GPUIntegerCoordinate = 0, GPUIntegerCoordinate = 0] GPUOrigin3D;
typedef [GPUIntegerCoordinate, GPUIntegerCoordinate, GPUIntegerCoordinate] GPUExtent3D;
```

I previously proposed this in gpuweb#299. After discussion, we compromised on
this union, and it was implemented in gpuweb#319. However there were a number
of voices who didn't like the added complexity (and perceived loss of
type expressivity), and since then I've come to agree that the unioned
version is not the best choice.
kainino0x added a commit to kainino0x/gpuweb that referenced this pull request Sep 16, 2020
This overload adds complexity without providing much benefit of clarity.
The semantics of the values in an array are clear according to context:

- 1d: [width]
- 1d-array: [width, arraysize]
- 2d: [width, height]
- 2d-array/cube/cube-array: [width, height, arraysize]
- 3d: [width, height, depth]

In order to implement this, this redefines GPUOrigin2D, GPUOrigin3D, and
GPUExtent3D from scratch, as if they were specified in the WebIDL spec.
These essentially implement a specific case of tuples with default
values, which could be expressed in pseudo-WebIDL as:

```
typedef [GPUIntegerCoordinate = 0, GPUIntegerCoordinate = 0] GPUOrigin2D;
typedef [GPUIntegerCoordinate = 0, GPUIntegerCoordinate = 0, GPUIntegerCoordinate = 0] GPUOrigin3D;
typedef [GPUIntegerCoordinate, GPUIntegerCoordinate, GPUIntegerCoordinate] GPUExtent3D;
```

I previously proposed this in gpuweb#299. After discussion, we compromised on
this union, and it was implemented in gpuweb#319. However there were a number
of voices who didn't like the added complexity (and perceived loss of
type expressivity), and since then I've come to agree that the unioned
version is not the best choice.
kainino0x added a commit to kainino0x/gpuweb that referenced this pull request Sep 16, 2020
This overload adds complexity without providing much benefit of clarity.
The semantics of the values in an array are clear according to context:

- 1d: [width]
- 1d-array: [width, arraysize]
- 2d: [width, height]
- 2d-array/cube/cube-array: [width, height, arraysize]
- 3d: [width, height, depth]

In order to implement this, this redefines GPUOrigin2D, GPUOrigin3D, and
GPUExtent3D from scratch, as if they were specified in the WebIDL spec.
These essentially implement a specific case of tuples with default
values, which could be expressed in pseudo-WebIDL as:

```
typedef [GPUIntegerCoordinate = 0, GPUIntegerCoordinate = 0] GPUOrigin2D;
typedef [GPUIntegerCoordinate = 0, GPUIntegerCoordinate = 0, GPUIntegerCoordinate = 0] GPUOrigin3D;
typedef [GPUIntegerCoordinate, GPUIntegerCoordinate, GPUIntegerCoordinate] GPUExtent3D;
```

I previously proposed this in gpuweb#299. After discussion, we compromised on
this union, and it was implemented in gpuweb#319. However there were a number
of voices who didn't like the added complexity (and perceived loss of
type expressivity), and since then I've come to agree that the unioned
version is not the best choice.
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
Reserves .vscode/ ("folder settings") for personal editor settings.
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.

3 participants