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

Proposal: remove Undefined from VertexStepMode and binding type enums #424

Closed
wants to merge 3 commits into from

Conversation

kainino0x
Copy link
Collaborator

@kainino0x kainino0x commented Nov 20, 2024

After @beaufortfrancois started implementing VertexBufferNotUsed/BindingNotUsed in Dawn I am reconsidering that we should have Undefined values for these enums at all. There is not really any value in them, since:

  • In C they are not the default so you would have to specify them explicitly anyway, at which point you may as well type Vertex or Uniform/Float/Filtering/WriteOnly
    • Implementations of JS over C will already be doing the trivial defaulting in the WebIDL layer, and wouldn't translate undefined to WGPUVertexStepMode_Undefined/etc.
  • In languages that have defaults, they are free to be clever and have e.g. WGPUBindGroupLayoutEntry.buffer.type default to BindingNotUsed but have WGPUBufferBindingLayout.type default to Uniform. Or just use algebraic data types, or whatever.

See also PR #364; issues #234, #242

EDIT: #450 is a possible alternative to this PR

@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Nov 20, 2024
@kainino0x
Copy link
Collaborator Author

kainino0x commented Nov 20, 2024

  • In languages that have defaults

Technically we do have "defaults" in C, in the form of the INIT structs. We could set the defaults in there but we still do not need Undefined to exist to be able to do that. Unless we think these structs will get reused in a different context with different defaults (seems vanishingly unlikely).

@eliemichel
Copy link
Collaborator

I very much approve this! Even in a pure C use case, I can see no use of the Undefined value whereever there is a NotUsed one, since in JS the semantics is the same (right?).

@kainino0x
Copy link
Collaborator Author

I very much approve this! Even in a pure C use case, I can see no use of the Undefined value whereever there is a NotUsed one, since in JS the semantics is the same (right?).

They don't map to the same JS:

  • C { .buffer={ .type=BindingNotUsed } } ↔ JS { buffer: undefined }
  • C { .buffer={ .type=Undefined } } ↔ JS { buffer: { type: undefined } }
  • However in practice that's exactly the same as doing:
    C { .buffer={ .type=Uniform } } ↔ JS { buffer: { type: "uniform" } }

@kainino0x
Copy link
Collaborator Author

kainino0x commented Nov 21, 2024

Technically we do have "defaults" in C, in the form of the INIT structs. We could set the defaults in there but we still do not need Undefined to exist to be able to do that. Unless we think these structs will get reused in a different context with different defaults (seems vanishingly unlikely).

To write that out, regardless of which way we go on removing Undefined, we could change the INIT macros from:

WGPU_BIND_GROUP_LAYOUT_ENTRY_INIT { ... /*.buffer=*/WGPU_BUFFER_BINDING_LAYOUT_INIT ... };
WGPU_BUFFER_BINDING_LAYOUT_INIT { ... /*.type=*/WGPUBufferBindingType_BindingNotUsed ... };

to

WGPU_BIND_GROUP_LAYOUT_ENTRY_INIT {
    ... /*.buffer=*/_wgpu_MAKE_INIT_STRUCT(WGPUBufferBindingLayout, {
      ... /*.type=*/WGPUBufferBindingType_BindingNotUsed ...
    }) ...
  };
WGPU_BUFFER_BINDING_LAYOUT_INIT { ... /*.type=*/WGPUBufferBindingType_Uniform ... };

so you can write:
{ .buffer=WGPU_BUFFER_BINDING_LAYOUT_INIT }
and have it do what { buffer: {} } does in JS.

@kainino0x
Copy link
Collaborator Author

In #427 I did think of one small reason we might keep Undefined:

  • Maybe INIT macros should prefer Undefined over trivial defaults e.g. /*.dimension=*/WGPUTextureDimension_2D in case the JS API changes them from trivial (WebIDL) defaults to non-trivial defaults (non-breakingly, but in such a way that they depend on the values of other new members or something).
    (I am not sure if we ever discussed this before. I know we said that we should implement all of WebIDL's trivial defaulting in webgpu.h implementations though.)

That last line above would instead be:

WGPU_BUFFER_BINDING_LAYOUT_INIT { ... /*.type=*/WGPUBufferBindingType_Undefined ... };

but the effect would be the same.

@kainino0x
Copy link
Collaborator Author

kainino0x commented Nov 23, 2024

Nov 21 meeting:

  • KN: Realized as François was implementing VertexBufferNotUsed/BindingNotUsed that Undefined is just… not useful in C. Remove? And in the INIT structs, should we set the defaults to NotUsed or to the real default?
  • Aside: Fix documentation on size/pointer pairs for array members #431
  • CF: Difference between a hole, and having 0 attributes?
  • KN: JS API probably requires a vertex buffer be set if you have 0 attributes, but I'm not sure.
  • CF: Could use attributeCount=0 to indicate a hole (move the sentinel to attributeCount)
  • to answer question: No, don't need Undefined
  • KN to investigate using attributeCount (what does the JS API do - and could it change to treat [] as a hole?)

@kainino0x kainino0x removed the !discuss Needs discussion (at meeting or online) label Nov 23, 2024
@kainino0x kainino0x enabled auto-merge (squash) November 23, 2024 03:01
@kainino0x kainino0x disabled auto-merge November 23, 2024 03:02
@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Nov 23, 2024
@kainino0x
Copy link
Collaborator Author

Ugh I forgot we should discuss the whole initialization thing I was talking about before I land this, will leave it on the agenda for now.

@kainino0x
Copy link
Collaborator Author

Moved that issue to #450.

@kainino0x kainino0x removed the !discuss Needs discussion (at meeting or online) label Dec 9, 2024
@kainino0x kainino0x closed this Dec 9, 2024
@kainino0x
Copy link
Collaborator Author

Closing in favor of #450

@kainino0x kainino0x deleted the no-undefined branch December 9, 2024 21:42
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.

2 participants