Make depthClearValue, depthWriteEnabled, depthCompare required - #3849
Conversation
This commit makes depthClearValue, depthWriteEnabled, and depthCompare required for V1 as discussed at gpuweb#3798 We might revisit them Post-V1.
|
Previews, as seen when this build job started (ee29660): |
Instead, check that it is provided if format has a depth aspect and depthLoadOp is clear in the validation.
| boolean depthWriteEnabled = false; | ||
| GPUCompareFunction depthCompare = "always"; | ||
| required boolean depthWriteEnabled; | ||
| required GPUCompareFunction depthCompare; |
There was a problem hiding this comment.
Hm... arguably for the same reason, maybe these should be required only if the format has a depth aspect?
There was a problem hiding this comment.
Though like with the other case it's annoying for implementations; especially so in this case, because this boolean becomes tri-state.
There was a problem hiding this comment.
That wouldn't be a problem if we made it an exception instead of a validation rule. But that would be a weird carveout.
There was a problem hiding this comment.
Hm... arguably for the same reason, maybe these should be required only if the format has a depth aspect?
Yeah, sounds like it is more consistent with depthClearValue although the implementation could be more complex as depthClearValue... If no objection, I will update the PR soon.
That wouldn't be a problem if we made it an exception instead of a validation rule.
Would you mind explaining it a bit more for me please because I couldn't get well? (Honestly I haven't comprehend the difference exception and validation rule well yet.)
There was a problem hiding this comment.
If the error is raised as an exception then it can be implemented at the JavaScript bindings level (ie: Blink in Chromium), which is quite a bit simpler. If it's done as a validation error it needs to be passed all the way through the WebGPU native implementation, which then needs to figure out things like how to handle a tri-state boolean in a C API.
The reasons for preferring a validation error are that it's more consistent with the handling of every other error that is raised from this part of the code, and there may be some cases (not here) where some of the information necessary to validate are only tracked in the WebGPU implementation.
There was a problem hiding this comment.
There's no state needed to validate this, but there is a bit of static info that I don't think we currently need in the content timeline: for a given format, whether it has a depth aspect or not.
Probably will land this and open another issue for this.
There was a problem hiding this comment.
Thanks for the explanations!
This commit makes depthWriteEnabled and depthCompare required and makes depthClearValue conditionally required for the spec change in WebGPU V1. gpuweb/gpuweb#3849 depthClearValue is required if depthLoadOp is clear and the attachment has a depth aspect. To simulate it, this commit lets NAN represent unspecified depthClearValue and lets the default value of depthClearValue be NAN. Bug: dawn:1669 Change-Id: I469338e909b1d3c345bc2642ee47daee858909ca Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/120620 Reviewed-by: Corentin Wallez <cwallez@chromium.org> Reviewed-by: Austin Eng <enga@chromium.org> Kokoro: Kokoro <noreply+kokoro@google.com> Commit-Queue: Corentin Wallez <cwallez@chromium.org>
This commit makes depthWriteEnabled and depthCompare required and makes depthClearValue conditionally required for the spec change in WebGPU V1. gpuweb/gpuweb#3849 Also see https://dawn-review.googlesource.com/c/dawn/+/120620 Bug: dawn:1669 Change-Id: Ie3a3bd5844309b353ee0403fbb8c3e8aa1ba1b82 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4270352 Commit-Queue: Austin Eng <enga@chromium.org> Reviewed-by: Kai Ninomiya <kainino@chromium.org> Cr-Commit-Position: refs/heads/main@{#1116657}
…ired. In particular, depthCompare no longer defaults to ALWAYS. gpuweb/gpuweb#3849
Related: #3777 #3798
This PR makes depthClearValue, depthWriteEnabled, and depthCompare required for V1 as discussed at #3798. We might revisit them Post-V1.