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.

Sign up for GitHub

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

Jump to bottom

Should SetBlendConstant take float,float,float,float instead of WGPUColor? #378

Closed
kainino0x opened this issue Oct 21, 2024 · 3 comments · May be fixed by #394
Closed

Should SetBlendConstant take float,float,float,float instead of WGPUColor? #378

kainino0x opened this issue Oct 21, 2024 · 3 comments · May be fixed by #394
Assignees
@kainino0x
Labels
needs exploration Needs offline exploration (e.g. in an implementation)

Comments

@kainino0x
Copy link
Collaborator

WGPUColor is set up to use double because it can represent all of the possible render pass clear values - it's used as a union of f32, i32, and u32.

However, AFAIK, blending, and therefore wgpuRenderPassEncoderSetBlendConstant, never applies to sint/uint type textures, so should we just take four 32-bit floats instead? Slightly more efficient.
@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Oct 21, 2024
@Kangz
Copy link
Collaborator

Kangz commented Oct 23, 2024

I checked that all APIs do take 32bit floats.

@kainino0x kainino0x mentioned this issue Nov 9, 2024
Draft
@kainino0x
Copy link
Collaborator Author

Dec 12 meeting:

  • KN: Worth simplifying? SetBlendConstant only needs f32, never i32/u32 (WGPUColor uses f64 to represent all three)
  • KN: Came up because some bindings might want to have some kind of union-like f32/i32/u32 for colors. WGPUColor designed to represent a color in a texture, which is not exactly what SetBlendConstant is.
  • (discussion of why to do this change)
    • CF: Could have WGPUColor32 of f32s.
  • CF: No real preference.
  • Easier to not change. But may think about it some more.
  • Not hard for Dawn to migrate, with C++ overloads.

@kainino0x kainino0x self-assigned this Dec 12, 2024
@kainino0x kainino0x added needs exploration Needs offline exploration (e.g. in an implementation) and removed !discuss Needs discussion (at meeting or online) labels Dec 12, 2024
@kainino0x
Copy link
Collaborator Author

Pretty sure this is not going to cause anything more than tiny differences at the binding level. But finally came up with a deciding factor: since the JS API takes GPUColor here (and therefore f64s outside of the range of f32) we should really just match JS so we don't introduce any more mismatches we have to deal with. Closing.

BTW I have filed gpuweb/gpuweb#5020 because thinking about this I realized we are actually missing a bunch of minutiae from the JS spec.

@kainino0x kainino0x closed this as not planned Won't fix, can't repro, duplicate, stale Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kainino0x kainino0x

Labels
needs exploration Needs offline exploration (e.g. in an implementation)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[proposal] Change SetBlendConstant to take 4 floats kainino0x/webgpu-headers
2 participants
@Kangz @kainino0x