Skip to content

For e.g. GPUOrigin2D, accept either dict or sequence.#319

Merged
kdashg merged 1 commit intogpuweb:masterfrom
kdashg:color-union
Jun 17, 2019
Merged

For e.g. GPUOrigin2D, accept either dict or sequence.#319
kdashg merged 1 commit intogpuweb:masterfrom
kdashg:color-union

Conversation

@kdashg
Copy link
Contributor

@kdashg kdashg commented Jun 6, 2019

Alternative to #299.

@kdashg kdashg requested review from RafaelCintron and kainino0x June 6, 2019 00:51
Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

OK with me.

@grovesNL
Copy link
Contributor

grovesNL commented Jun 7, 2019

It seems better to only expose one of these variations for consistency – users will naturally tend towards one of these anyway.

Personally I don't mind some verbosity here (I think the explicitness is useful) so I prefer the dict approach. But I think I'd still prefer the sequence alternative over having both.

@kvark
Copy link
Contributor

kvark commented Jun 7, 2019

In agreement with @grovesNL , I'm also wondering what this dichotomy is going to look like in a C header?

@kdashg
Copy link
Contributor Author

kdashg commented Jun 7, 2019

The C header is not bound by the ergonomic choices of the WebIDL.

@kainino0x
Copy link
Contributor

I agree, I think in C always using a struct here would be fine. Inline initialization would look the same as an array ({0, 0, 0, 0}) in C/C++.

@kdashg
Copy link
Contributor Author

kdashg commented Jun 17, 2019

Agreed on the 2019June17 call.

@kdashg kdashg merged commit 32dc150 into gpuweb:master Jun 17, 2019
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
* Allow marking tests as explicitly unimplemented

This allows for test structure to be committed into the CTS before the
tests themselves are implemented.

* Make the testCreationStack slightly nicer
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.

5 participants