Bug 202031

Summary: Web Inspector: Canvas: modifications to shader modules can be shared between vertex/fragment shaders
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, jonlee, justin_fan, keith_miller, mark.lam, mmaxfield, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 201675    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Devin Rousso
Reported 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
Attachments
Patch (13.26 KB, patch)
2019-09-20 00:25 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (28.96 KB, patch)
2019-10-08 00:26 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (28.42 KB, patch)
2019-10-08 00:29 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (41.74 KB, patch)
2019-10-08 12:52 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (47.37 KB, patch)
2019-10-08 14:43 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2019-09-20 00:25:54 PDT
Created attachment 379219 [details] Patch
Devin Rousso
Comment 2 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.
Devin Rousso
Comment 3 2019-10-07 16:40:01 PDT Comment hidden (obsolete)
Devin Rousso
Comment 4 2019-10-07 16:40:31 PDT Comment hidden (obsolete)
Devin Rousso
Comment 5 2019-10-08 00:26:21 PDT
Created attachment 380408 [details] Patch Needs tests, but the UI is all there
Devin Rousso
Comment 6 2019-10-08 00:29:38 PDT
Created attachment 380409 [details] Patch Rebase (still needs tests)
EWS Watchlist
Comment 7 2019-10-08 00:30:19 PDT Comment hidden (obsolete)
Devin Rousso
Comment 8 2019-10-08 12:52:38 PDT
Created attachment 380452 [details] Patch
Devin Rousso
Comment 9 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 :(
Dean Jackson
Comment 10 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? :)
Devin Rousso
Comment 11 2019-10-08 14:43:24 PDT
Created attachment 380467 [details] Patch
Devin Rousso
Comment 12 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
Devin Rousso
Comment 13 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 :|
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2019-10-08 16:16:08 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16 2019-10-08 16:27:01 PDT
<rdar://problem/56097158>
Note You need to log in before you can comment on or make changes to this bug.