depthCompare is not required for depth attachments if not used #3069

beaufortfrancois · 2023-10-16T14:33:52Z

Following #3066, this PR adds tests that check that depthCompare is not required for formats with depth if it's not used. See gpuweb/gpuweb#4336

Requirements for PR author:

All missing test coverage is tracked with "TODO" or .unimplemented().
New helpers are /** documented */ and new helper files are found in helper_index.txt.
Test behaves as expected in a WebGPU implementation. (If not passing, explain above.) They pass in my Chromium build patched with https://dawn-review.googlesource.com/c/dawn/+/155703

Requirements for reviewer sign-off:

Tests are properly located in the test tree.
Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

@Kangz Please have a look

beaufortfrancois · 2023-10-16T14:34:41Z

src/webgpu/api/validation/render_pipeline/depth_stencil_state.spec.ts

+    const success =
+      // depthCompare is optional for stencil-only formats.
+      (!info.depth && depthWriteEnabled !== true) ||
+      // depthCompare is optional for formats with a depth if it is not used for anything.


I wish there was a way to make this code more readable but I didn't find it.

Often we use a pattern like

let success = true; if (...) success = false; if (...) success = false;

Most of the PR looks good, but I hope you don't mind if I'll finish the review after you've done a refactor like that. It's quite hard to read right now.

I think it can looks something approximately like:

let success = false; if (info.depth) { } else { if (depthWriteEnabled === false && areDepthFailOpKeep) success = true; if (depthCompare) { if (...) success = false; } }

Following your suggestion, I've uploaded another patch. Thank you @kainino0x!

Kangz · 2023-10-16T14:35:52Z

Sorry, I won't be able to take a look any time soon. Could you find a different reviewer?

beaufortfrancois · 2023-10-16T14:36:38Z

@kainino0x Please review this PR.

kainino0x · 2023-10-17T02:43:45Z

Pushed rebase since #3066 landed.

github-actions · 2023-10-17T02:56:01Z

Previews, as seen when this build job started (e94df2e):
Run tests | View tsdoc

github-actions · 2023-10-17T08:07:04Z

Previews, as seen when this build job started (923f09c):
Run tests | View tsdoc

kainino0x · 2023-10-18T16:55:04Z

src/webgpu/api/validation/render_pipeline/depth_stencil_state.spec.ts

+    let success = false;
+    if (!info.depth) {
+      // depthCompare is optional for stencil-only formats.
+      if (!depthWriteEnabled) success = true;


nit: This confused me a little because depthWriteEnabled=true is not even allowed with stencil-only formats. Then I realized this check is here because of that. Digging further it's quite hard to see how all of the validation rules from the spec appear here. [continued in other comment]

kainino0x · 2023-10-18T17:45:37Z

src/webgpu/api/validation/render_pipeline/depth_stencil_state.spec.ts

+    const areDepthFailOpKeep =
+      stencilFrontDepthFailOp === 'keep' && stencilBackDepthFailOp === 'keep';
+
+    let success = false;
+    if (!info.depth) {
+      // depthCompare is optional for stencil-only formats.
+      if (!depthWriteEnabled) success = true;
+    } else {
+      // depthCompare is optional for formats with a depth if it's not used for anything.
+      if (depthWriteEnabled === false && areDepthFailOpKeep) success = true;
+      if (depthCompare) {
+        // validation will succeed normally for formats with depth aspect.
+        if (depthWriteEnabled && areDepthFailOpKeep) success = true;
+        // validation will succeed as well for formats with stencil and depth aspect.
+        if (info.stencil && depthWriteEnabled !== undefined) success = true;
+      }
+    }


Apologies for another big change. It is much easier to think about written this way, but I still find it quite hard to pick apart where the different rules from the spec appear here. (I probably shouldn't have suggested inverting the usual pattern, as I realize there's a good reason we usually have let success = true; if (...) success = false.)

I think the most readable form would stick close to the spec:

Suggested change

const areDepthFailOpKeep =

stencilFrontDepthFailOp === 'keep' && stencilBackDepthFailOp === 'keep';

let success = false;

if (!info.depth) {

// depthCompare is optional for stencil-only formats.

if (!depthWriteEnabled) success = true;

} else {

// depthCompare is optional for formats with a depth if it's not used for anything.

if (depthWriteEnabled === false && areDepthFailOpKeep) success = true;

if (depthCompare) {

// validation will succeed normally for formats with depth aspect.

if (depthWriteEnabled && areDepthFailOpKeep) success = true;

// validation will succeed as well for formats with stencil and depth aspect.

if (info.stencil && depthWriteEnabled !== undefined) success = true;

}

}

const depthFailOpsAreKeep =

stencilFrontDepthFailOp === 'keep' && stencilBackDepthFailOp === 'keep';

const stencilStateIsDefault = depthFailOpsAreKeep;

let success = true;

if (depthWriteEnabled || depthCompare !== 'always') {

if (!info.depth) success = false;

}

if (!stencilStateIsDefault) {

if (!info.stencil) success = false;

}

if (info.depth) {

if (depthWriteEnabled === undefined) success = false;

if (depthWriteEnabled || !depthFailOpsAreKeep) {

if (depthCompare === undefined) success = false;

}

}

I would need you to test this to make sure it's right, but it follows the spec very closely so I hope it is.

@kainino0x Thanks for the detailed answer. I followed your last suggestion with one nit:

- if (depthWriteEnabled || depthCompare !== 'always') { + if (depthWriteEnabled || (depthCompare && depthCompare !== 'always')) {

I'm updating spec to make it clear as well. See gpuweb/gpuweb#4345

@kainino0x (gentle ping)

github-actions · 2023-10-19T08:28:12Z

Previews, as seen when this build job started (110265d):
Run tests | View tsdoc

kainino0x

LGTM!

beaufortfrancois commented Oct 16, 2023

View reviewed changes

beaufortfrancois requested a review from kainino0x October 16, 2023 14:36

Base automatically changed from depth to main October 17, 2023 01:48

kainino0x force-pushed the depthCompare branch from 63ec603 to e94df2e Compare October 17, 2023 02:43

beaufortfrancois added 2 commits October 17, 2023 09:52

depthCompare is not required for depth attachments if not used

8b87204

Refactor for success

923f09c

beaufortfrancois force-pushed the depthCompare branch from e94df2e to 923f09c Compare October 17, 2023 07:56

kainino0x reviewed Oct 18, 2023

View reviewed changes

Refactor success definition

110265d

kainino0x approved these changes Oct 24, 2023

View reviewed changes

kainino0x merged commit 73bcf42 into main Oct 24, 2023
2 checks passed

kainino0x deleted the depthCompare branch October 24, 2023 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

depthCompare is not required for depth attachments if not used #3069

depthCompare is not required for depth attachments if not used #3069

beaufortfrancois commented Oct 16, 2023 •

edited by kainino0x

Loading

beaufortfrancois Oct 16, 2023

kainino0x Oct 17, 2023

kainino0x Oct 17, 2023 •

edited

Loading

beaufortfrancois Oct 17, 2023

Kangz commented Oct 16, 2023

beaufortfrancois commented Oct 16, 2023

kainino0x commented Oct 17, 2023

github-actions bot commented Oct 17, 2023

github-actions bot commented Oct 17, 2023

kainino0x Oct 18, 2023

kainino0x Oct 18, 2023

beaufortfrancois Oct 19, 2023 •

edited

Loading

beaufortfrancois Oct 24, 2023

github-actions bot commented Oct 19, 2023

kainino0x left a comment

depthCompare is not required for depth attachments if not used #3069

depthCompare is not required for depth attachments if not used #3069

Conversation

beaufortfrancois commented Oct 16, 2023 • edited by kainino0x Loading

beaufortfrancois Oct 16, 2023

Choose a reason for hiding this comment

kainino0x Oct 17, 2023

Choose a reason for hiding this comment

kainino0x Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

beaufortfrancois Oct 17, 2023

Choose a reason for hiding this comment

Kangz commented Oct 16, 2023

beaufortfrancois commented Oct 16, 2023

kainino0x commented Oct 17, 2023

github-actions bot commented Oct 17, 2023

github-actions bot commented Oct 17, 2023

kainino0x Oct 18, 2023

Choose a reason for hiding this comment

kainino0x Oct 18, 2023

Choose a reason for hiding this comment

beaufortfrancois Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

beaufortfrancois Oct 24, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 19, 2023

kainino0x left a comment

Choose a reason for hiding this comment

beaufortfrancois commented Oct 16, 2023 •

edited by kainino0x

Loading

kainino0x Oct 17, 2023 •

edited

Loading

beaufortfrancois Oct 19, 2023 •

edited

Loading