Bug 202031 - Web Inspector: Canvas: modifications to shader modules can be shared between vertex/fragment shaders
Summary: Web Inspector: Canvas: modifications to shader modules can be shared between ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 201675
Blocks:
  Show dependency treegraph
 
Reported: 2019-09-19 22:36 PDT by Devin Rousso
Modified: 2019-10-08 16:27 PDT (History)
15 users (show)

See Also:


Attachments
Patch (13.26 KB, patch)
2019-09-20 00:25 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (28.96 KB, patch)
2019-10-08 00:26 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (28.42 KB, patch)
2019-10-08 00:29 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (41.74 KB, patch)
2019-10-08 12:52 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (47.37 KB, patch)
2019-10-08 14:43 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-09-19 22:36:50 PDT
# STEPS TO REPRODUCE:
1. inspect <https://webkit.org/demos/webgpu/compute-boids.html>
2. select Render Pipeline 1 (the same shader module is used for the vertex and fragment shaders)
3. edit the vertex shader
4. refresh both shaders
 => the fragment shaders update to reflect the changes made to the vertex shader
Comment 1 Devin Rousso 2019-09-20 00:25:54 PDT
Created attachment 379219 [details]
Patch
Comment 2 Devin Rousso 2019-09-24 22:14:54 PDT
Comment on attachment 379219 [details]
Patch

After speaking with Saam, he suggested that if the vertex and fragment shaders both shared the same shader module, we shouldn't use a split content view and instead update both the vertex and fragment shaders at the same time.  I think this is a great idea, as it handles the case where there are utility functions that are shared between the vertex and fragment shaders, so that any edits are shared.
Comment 3 Devin Rousso 2019-10-07 16:40:01 PDT Comment hidden (obsolete)
Comment 4 Devin Rousso 2019-10-07 16:40:31 PDT Comment hidden (obsolete)
Comment 5 Devin Rousso 2019-10-08 00:26:21 PDT
Created attachment 380408 [details]
Patch

Needs tests, but the UI is all there
Comment 6 Devin Rousso 2019-10-08 00:29:38 PDT
Created attachment 380409 [details]
Patch

Rebase (still needs tests)
Comment 7 Build Bot 2019-10-08 00:30:19 PDT Comment hidden (obsolete)
Comment 8 Devin Rousso 2019-10-08 12:52:38 PDT
Created attachment 380452 [details]
Patch
Comment 9 Devin Rousso 2019-10-08 14:29:00 PDT
Comment on attachment 380452 [details]
Patch

Ah, I forgot to adjust the test expectations so that 'inspector/canvas/updateShader-webgpu-sharedVertexFragment.html' doesn't get run on non-WebGPU-supporting platforms :(
Comment 10 Dean Jackson 2019-10-08 14:34:26 PDT
Comment on attachment 380452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380452&action=review

> Source/WebCore/inspector/InspectorShaderProgram.cpp:283
> +    auto payload = Inspector::Protocol::Canvas::ShaderProgram::create()
> +        .setProgramId(m_identifier)
> +        .setProgramType(programType.value())
> +        .setCanvasId(m_canvas.identifier())
> +        .release();

Did you swap to Java here? :)
Comment 11 Devin Rousso 2019-10-08 14:43:24 PDT
Created attachment 380467 [details]
Patch
Comment 12 Devin Rousso 2019-10-08 14:44:20 PDT
Comment on attachment 380452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380452&action=review

>> Source/WebCore/inspector/InspectorShaderProgram.cpp:283
>> +        .release();
> 
> Did you swap to Java here? :)

lol.  This is typical Web Inspector style :P
The code that generates these objects is pretty awesome too!  Makes some really clever usage of C++ templates to ensure that the required properties get added in the right order =D
Comment 13 Devin Rousso 2019-10-08 15:23:36 PDT
Comment on attachment 380467 [details]
Patch

I'm cq+ this now, since bugzilla EWS won't run any of the WebGPU tests anyways :|
Comment 14 WebKit Commit Bot 2019-10-08 16:16:06 PDT
Comment on attachment 380467 [details]
Patch

Clearing flags on attachment: 380467

Committed r250874: <https://trac.webkit.org/changeset/250874>
Comment 15 WebKit Commit Bot 2019-10-08 16:16:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-10-08 16:27:01 PDT
<rdar://problem/56097158>