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

Naming conventions for extension structs and implementation-specific extensions #212

Open
kainino0x opened this issue Aug 10, 2023 · 25 comments
Labels
extensibility Adding features without breaking API changes has resolution Issue is resolved, just needs to be done naming Bikeshedding names non-breaking Does not require a breaking change (that would block V1.0)

Comments

@kainino0x
Copy link
Collaborator

kainino0x commented Aug 10, 2023

We should define some naming conventions for both standardized extension structs and implementation-specific extensions. Right now we have the following (inconsistent) names:

WGPUPrimitiveState extensions:
WGPUPrimitiveDepthClipControl

WGPURenderPassDescriptor extensions:
WGPURenderPassDescriptorMaxDrawCount

WGPUShaderModuleDescriptor extensions:
WGPUShaderModuleSPIRVDescriptor
WGPUShaderModuleWGSLDescriptor

WGPUSurfaceDescriptor extensions:
WGPUSurfaceDescriptorFromAndroidNativeWindow
WGPUSurfaceDescriptorFromCanvasHTMLSelector
WGPUSurfaceDescriptorFromMetalLayer
WGPUSurfaceDescriptorFromWaylandSurface
WGPUSurfaceDescriptorFromWindowsHWND
WGPUSurfaceDescriptorFromXcbWindow
WGPUSurfaceDescriptorFromXlibWindow

Additionally for Dawn we might want things like WGPUSomeDescriptorNewFeatureDAWN? (though idk what the naming would be for wgpu-native)

@kainino0x kainino0x added !discuss Needs discussion (at meeting or online) naming Bikeshedding names labels Aug 10, 2023
@Kangz
Copy link
Collaborator

Kangz commented Aug 11, 2023

For ergonomics it seems that typing Descriptor all the time is annoying. So I'd favor a rule like:

  • Take the name of the chain root, remove Descriptor, State or other common words
  • Optionally add a name to scope to a project / vendor Dawn, Wgpu, ...
  • Add the extra words.

So the list would become:

WGPUPrimitiveState extensions:
WGPUPrimitiveDepthClipControl

WGPURenderPassDescriptor extensions:
WGPURenderPassMaxDrawCount

WGPUShaderModuleDescriptor extensions:
WGPUShaderModuleSPIRV
WGPUShaderModuleWGSL

WGPUSurfaceDescriptor extensions:
WGPUSurfaceFromAndroidNativeWindow
WGPUSurfaceFromCanvasHTMLSelector
WGPUSurfaceFromMetalLayer
WGPUSurfaceFromWaylandSurface
WGPUSurfaceFromWindowsHWND
WGPUSurfaceFromXcbWindow
WGPUSurfaceFromXlibWindow

WDYT?

@kainino0x
Copy link
Collaborator Author

kainino0x commented Aug 17, 2023

webgpu.h meeting resolution (OUTDATED):

  • fold DepthClipControl into PrimitiveState
  • drop Descriptor from chained struct names as proposed above
  • add Wgpu or Dawn at the end for implementation-specific extensions

See also #214 about enum range reservations.

@kainino0x kainino0x added has resolution Issue is resolved, just needs to be done extensibility Adding features without breaking API changes and removed !discuss Needs discussion (at meeting or online) labels Aug 17, 2023
@kainino0x
Copy link
Collaborator Author

Feedback that eliding "Descriptor" from the extension struct names is confusing:
https://chromium-review.googlesource.com/c/chromium/src/+/5691772/comment/25f7fc96_8d6b6f12/

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

kainino0x commented Jul 18, 2024

Jul 18 meeting:

  • Got feedback that new names are hard to understand
  • WGPUSurfaceFromCanvasHTMLSelectorDescriptor
  • WGPUSurfaceFromCanvasHTMLSelector
  • WGPUSurfaceDescriptorFromCanvasHTMLSelector
  • WGPUSurfaceDescriptor_FromCanvasHTMLSelector
    • With this in C++ we could actually namespace it?
  • KN: Try to give them descriptive names that don't depend on the descriptor they're attached to? (Docs tell you what they're attached to.)
  • CF: Vulkan style.
  • Brainstorm:
  • WGPUCanvasHTMLSelector
  • WGPUExtCanvasHTMLSelector
  • WGPUSurfaceSourceCanvasHTMLSelector
  • WGPUSurfaceSourceAndroidNativeWindow
  • WGPUSurfaceSourceMetalLayer
  • WGPUShaderSourceWGSL
  • WGPUShaderSourceSPIRV <- current caps style
  • WGPUShaderSourceSPIRV_EXT
  • WGPUShaderSourceGLSLWgpu
  • WGPUShaderSourceGLSL_Wgpu ?? doesn’t look right
  • WGPUShaderSourceGLSL_WGPU
  • WGPUShaderSourceGLSL_wgpu ?? Feels like typo
  • WGPUShaderSourceGLSL_DAWN
  • WGPUShaderSourceGLSL_GFXRS
  • WGPUShaderSourceGLSL_RUSTYGRAPHICSMAGES
  • WGPUShaderSourceGLSL_Dawn
  • WGPUShaderSourceGLSL_NATIVE
  • WGPUShaderSourceGLSL_STD
  • WGPUShaderSourceGLSL_EMSCRIPTEN ??
  • WGPUShaderSourceGlslWgpu <- proposed caps style?
  • Propose the naming conventions in bold above, get feedback
    • Also need to update #214

(I'll expand out that proposal in a followup comment)

@kainino0x kainino0x removed !discuss Needs discussion (at meeting or online) has resolution Issue is resolved, just needs to be done labels Jul 18, 2024
@kainino0x
Copy link
Collaborator Author

kainino0x commented Jul 18, 2024

Rough proposal:

WGPURenderPassDescriptor extensions:
WGPURenderPassMaxDrawCount or WGPUMaxDrawCount

WGPUShaderModuleDescriptor extensions:
WGPUShaderSourceSPIRV
WGPUShaderSourceWGSL

WGPUSurfaceDescriptor extensions:
WGPUSurfaceSourceAndroidNativeWindow
WGPUSurfaceSourceCanvasHTMLSelector_Emscripten (see #214 (comment))
WGPUSurfaceSourceMetalLayer
WGPUSurfaceSourceWaylandSurface
WGPUSurfaceSourceWindowsHWND
WGPUSurfaceSourceXcbWindow or WGPUSurfaceSourceXCBWindow
WGPUSurfaceSourceXlibWindow

Other examples of how extensions could be named:

WGPUPrimitiveState extensions:
WGPUPrimitiveDepthClipControl or WGPUDepthClipControl (we are getting rid of this but this is what it would look like)

WGPUShaderModuleDescriptor extensions:
WGPUShaderSourceGLSL_WGPU
WGPUShaderSourceGLSL_Dawn

@beaufortfrancois
Copy link
Contributor

WGPUPrimitiveState extensions:
WGPUPrimitiveDepthClipControl or WGPUDepthClipControl

My understanding was that we'd fold DepthClipControl into PrimitiveState with #311
@kainino0x Did I misunderstand?

@Kangz
Copy link
Collaborator

Kangz commented Jul 22, 2024

Yes we're folding it, but that's just for the explanation of the new naming rules.

@kainino0x
Copy link
Collaborator Author

Oh yes, I intended to note that but forgot. Edited above.

@beaufortfrancois
Copy link
Contributor

I believe we can close this issue after #320 gets merged.

@kainino0x
Copy link
Collaborator Author

You're right, thanks!
Dawn bug is https://crbug.com/42241174

@kainino0x kainino0x added the has resolution Issue is resolved, just needs to be done label Aug 30, 2024
@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Oct 18, 2024
@kainino0x kainino0x reopened this Oct 18, 2024
@kainino0x
Copy link
Collaborator Author

Applying this naming convention to Dawn we are finding we don't know how to name:

  • New enums, and their enum values.
    WGPUMyEnumType_Dawn
    WGPUMyEnumType_Dawn_MyValue? suffix on enum name, but it ends up in the middle.
    WGPUMyEnumType_MyValue_Dawn? suffix at the end of each thing ⭐
    WGPUMyEnumType_Dawn_MyValue_Dawn? (lol)
    wgpu::MyEnumType_Dawn::MyValue?
    wgpu::dawn::MyEnumType::MyValue? ⭐

  • Methods of new objects and FreeMembers functions of new structs
    WGPUAdapterPropertiesMemoryHeaps
    wgpuAdapterPropertiesMemoryHeaps_DawnFreeMembers()?
    wgpuAdapterPropertiesMemoryHeapsFreeMembers_Dawn()? ⭐

    WGPUMyObject_Dawn
    wgpuMyObject_DawnMyMethod()?? (bad)
    wgpuMyObjectMyMethod_Dawn()? ⭐
    wgpu::MyObject_Dawn::MyMethod()?
    wgpu::dawn::MyObject::MyMethod()? ⭐

Marked the ones I like personally with ⭐.

@Kangz
Copy link
Collaborator

Kangz commented Oct 21, 2024

Adding the namespace in C++ is cool, but could be mildly difficult to do in Dawn, though it's not breaking for webgpu.h so it's ok to do later. That said I really don't understand why we need a _ to separate the extension block name from the rest of the name. There are likely to be very few blocks, and they will have clear names like <vendor> or "Native". IMHO it should be a prefix to go from general (WGPU) to more specific. The block is the second most general thing after WGPU. Subjectively it makes things easy to ready while not looking weird with random underscores:

New enums and their enum values:

  • WGPUDawnMyEnumType
  • WGPUDawnMyEnumType_MyValue
  • WGPUEnumType_DawnMyValue
  • wgpu::DawnEnumType
  • wgpu::DawnEnumType::MyValue
  • wgpu::EnumType::DawnMyValue
  • Alternatively we could do the namespacing in C++ at some point wgpu::dawn::EnumType::MyValue.

New methods and new objects

  • WGPUDawnAdapterPropertiesMemoryHeaps
  • wgpuDawnAdapterPropertiesMemoryHeaps_FreeMembers()
  • WGPUDawnMyObject
  • wgpuDawnMyObject_MyMethod()
  • wgpuObjectType_DawnMethod()
  • wgpu::DawnMyObject
  • wgpu::DawnMyObject::MyMethod()
  • wgpu::MyObject::DawnMethod()
  • Likewise we could namespace in C++ wgpu::dawn::MyObject.

@kainino0x
Copy link
Collaborator Author

IMHO it should be a prefix to go from general (WGPU) to more specific.

The Khronos-like style makes it so when something gets promoted to core, only the suffix is removed. I don't have that much preference, but we do have the problem that one of the blocks is for wgpu so we would have WGPUWgpuFoo and wgpuWgpuBar()

@kainino0x
Copy link
Collaborator Author

kainino0x commented Oct 29, 2024

wgpu-native's suffix could maybe change to wgpurs or gfxrs or something. (or wgpunative but that's still very confusing)

@Kangz
Copy link
Collaborator

Kangz commented Oct 31, 2024

TBH wgpuWgpu is weird but not a big deal.

@beaufortfrancois
Copy link
Contributor

Just wanted to check in and see if we've decided on a naming convention yet.

@eliemichel
Copy link
Collaborator

FWIW, my personal opinion: Spontaneously I would have opted for @Kangz' proposal (and agree that a wgpuWgpu prefix is only a minor issue, and really not the only case where having the project called "wgpu" is confusing). I see the point about the Khronos-like style, but is it used anywhere else in WebGPU, or are there any specific reasons to adopt it? How does Vulkan handle naming for extensions that add enum values, or new enums?

@kainino0x
Copy link
Collaborator Author

Nov 25 meeting:

  • almost totally missed putting this on the agenda
  • discussion: (definitely don't want double suffix or suffix stuck in the middle)
  • OK with either, no real opinion:
    • WGPUDawnMyEnumType + WGPUDawnMyEnumType_MyValue
    • WGPUMyEnumType_Dawn + WGPUMyEnumType_MyValue_Dawn

Dawn hasn't implemented suffixes yet, and I checked and wgpu-native has not yet either.
(Dawn naming is ad hoc; wgpu-native has its extensions in a separate header.)

So @Kangz since you prefer WGPUDawnMyEnumType_MyValue we may as well go with that (wgpu-native will probably use WGPUWgpuMyEnumType_MyValue)


I see the point about the Khronos-like style, but is it used anywhere else in WebGPU, or are there any specific reasons to adopt it?

The only specific reason I see to adopt it is that the un-suffixed version of the name (if something becomes core) is exactly the same except for the very end. But that's not significantly different from removing the prefix between WGPU and the rest of the name. Otherwise just because it is familiar to GL/Vulkan developers.

How does Vulkan handle naming for extensions that add enum values, or new enums?

I was pretty sure about this but I should have checked :) Vulkan does this when adding a new enum:
VkPerformanceCounterUnitKHR
VK_PERFORMANCE_COUNTER_UNIT_GENERIC_KHR

I suppose we could have inferred we wanted to use that style from the fact that we said "vulkan style", but we came to a different conclusion anyway, so no problem.

@kainino0x
Copy link
Collaborator Author

Updated #214 accordingly

@kainino0x
Copy link
Collaborator Author

kainino0x commented Nov 26, 2024

Just thinking a little bit more about adding new values to existing enums and new methods to existing objects, they still get sandwiched which I like a bit less than having suffixes always at the end.

Also wanted to point out:

  • wgpuDawnAdapterPropertiesMemoryHeaps_FreeMembers()
  • wgpuDawnMyObject_MyMethod()
  • wgpuObjectType_DawnMethod()

We don't currently separate the type name from the method name so this would look instead like

  • wgpuDawnAdapterPropertiesMemoryHeapsFreeMembers()
  • wgpuDawnMyObjectMyMethod()
  • wgpuObjectTypeDawnMyMethod() <- fine but not the best

... unless we insert the underscores specifically for the case where we are adding methods to existing objects.

@Kangz
Copy link
Collaborator

Kangz commented Nov 26, 2024

Yeah I think wgpuBufferDawnClearWithDouble (or w/e) is pretty clear that it's on WGPUBuffer and a Dawn method name. The examples provided above don't use any concrete name which makes it harder to separate the parts, but with wgpuNounDawnVerb it's pretty clear what's happening.

@kainino0x
Copy link
Collaborator Author

Ack.

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

kainino0x commented Nov 27, 2024

To go through everything, for the record, does this seem right?

wgpuPrefixNewFunction

WGPUPrefixNewObject
wgpuPrefixNewObjectNewMethod
wgpuOldObjectPrefixNewMethod

WGPUPrefixNewEnum
WGPUPrefixNewEnum_OldValue
WGPUOldEnum_PrefixNewValue

WGPUPrefixNewStruct

WGPUPrefixNewBitflagType
WGPUPrefixNewBitflagType_NewValue
WGPUOldBitflagType_PrefixNewValue

WGPUPrefixNewCallback

WGPU_PREFIX_NEW_CONSTANT

@eliemichel
Copy link
Collaborator

To go through everything, for the record, does this seem right?

It does!

@kainino0x
Copy link
Collaborator Author

I totally forgot the code generator actually implements the suffixing, so we need to fix that. (It's not used in this repo so it doesn't affect the core webgpu.h.)

@kainino0x kainino0x reopened this Dec 3, 2024
@kainino0x kainino0x added the non-breaking Does not require a breaking change (that would block V1.0) label Dec 3, 2024
eliemichel added a commit to eliemichel/webgpu-headers that referenced this issue Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extensibility Adding features without breaking API changes has resolution Issue is resolved, just needs to be done naming Bikeshedding names non-breaking Does not require a breaking change (that would block V1.0)
Projects
None yet
Development

No branches or pull requests

4 participants