Add validation rule for QuerySet and Timestamp Query - #1161
Conversation
Improve the description and validation rule for: - GPUQuerySet and GPUQuerySetDescriptor - writeTimestamp on GPUCommandEncoder, GPUComputePassEncoder, GPURenderPassEncoder.
kainino0x
left a comment
There was a problem hiding this comment.
Looking pretty good so far! A few small comments.
Kangz
left a comment
There was a problem hiding this comment.
Overall validation logic looks good, but I think we might want to have more explicit logic to track the queries used in the render pass. Maybe it could be a follow up PR to unblock this one.
kvark
left a comment
There was a problem hiding this comment.
LGTM. We can argue whether some pieces are better written formally (via internal slots and equations), this is better left to the editor meeting :)
One question about the contents though
kainino0x
left a comment
There was a problem hiding this comment.
Sorry for the slow review, overall LGTM
Co-authored-by: Kai Ninomiya <kainino1@gmail.com>
Oh, I missed this one. Co-authored-by: Kai Ninomiya <kainino1@gmail.com>
Co-authored-by: Kai Ninomiya <kainino1@gmail.com>
36ce9b1 to
3b76112
Compare
3b76112 to
f10d48b
Compare
kainino0x
left a comment
There was a problem hiding this comment.
FYI: force pushes make it hard to figure out what the diff is since my last review, so prefer to just make additional small commits instead, and reserve force-pushes for rebases. (Or just use merge commits instead of rebases when needed.)
Not really a problem for this PR right now since the final PR is small enough I can just look at the whole thing :)
Co-authored-by: Kai Ninomiya <kainino1@gmail.com>
| **Called on:** {{GPURenderPassEncoder}} |this|. | ||
|
|
||
| **Arguments:** | ||
| <pre class=argumentdef for="GPURenderPassEncoder/writeTimestamp(querySet, queryIndex)"> |
There was a problem hiding this comment.
did we consider a partial interface having this stuff, so that we can specify it once instead of repeating ourselves for every type of pass and the command encoder?
There was a problem hiding this comment.
I don't think we have closely considered it (a mixin, I think you mean). I guess it depends on whether we expect anything in the algorithm to differ between them, but if not, we should probably try to do it.
…uweb#1161) * Add validation,resource_usages,texture,in_render_misc:* - Part II This patch adds the second part of the below test: validation,resource_usages,texture,in_render_misc:* - subresources,set_bind_group_on_same_index_depth_stencil_texture * Address reviewer's comments
Improve the description and validation rule for:
Preview | Diff