Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(webgpu): Fix support of WebGPU render backend #2816

Merged
merged 4 commits into from
Oct 26, 2023

Conversation

finetjul
Copy link
Member

@finetjul finetjul commented May 5, 2023

Context

Running the LineWidget example with WebGPU backend generated an error because WidgetManager requires onWindowResizeEvent public function.
Moreover in many places getOpenGLRenderWindow() was still called (even in a WebGPU context)

Results

Expose onWindowResizeEvent and trigger it when the size of the window is modified.
Rename getOpenGLRenderWindow() into getApiSpecificRenderWindow()

Changes

  • Documentation and TypeScript definitions were updated to match those changes

PR and Code Checklist

  • semantic-release commit messages
  • Run npm run reformat to have correctly formatted code

Testing

  • This change adds or fixes unit tests
  • Tested environment:
    • vtk.js: master
    • OS: Windows 11
    • Browser: Chrome Canary

@finetjul
Copy link
Member Author

finetjul commented May 5, 2023

@floryst there is still an error when mousing over the render window. There are many calls to getSelectedDataForXY, eventually model._capturedBuffers is null and model._capturedBuffers.generateSelection() fails.

You must have had that error that would explain why you added the check callID !== model._currentUpdateSelectionCallID.
Any chance you know what is missing here ?

@floryst
Copy link
Collaborator

floryst commented May 8, 2023

captureBuffers is the issue here. The first call to captureBuffers sets _captureInProgress to true. Subsequent calls (e.g. mouse movement) will see that _captureInProgress == true, so they return early. Importantly, they return when there are no captured buffers.

A quick fix would be the following implementation. I don't know if there are any unintended effects, however. For instance, I don't know if we would need to worry if x1, y1, x2, y2 don't correspond to the current captureInProgress.

@@ -202,22 +202,23 @@ function vtkWidgetManager(publicAPI, model) {
 
   async function captureBuffers(x1, y1, x2, y2) {
     if (model._captureInProgress) {
+      await model._captureInProgress;
       return;
     }
-    model._captureInProgress = true;
     renderPickingBuffer();
 
     model._capturedBuffers = null;
-    model._capturedBuffers = await model._selector.getSourceDataAsync(
+    model._captureInProgress = model._selector.getSourceDataAsync(
       model._renderer,
       x1,
       y1,
       x2,
       y2
     );
+    model._capturedBuffers = await model._captureInProgress;
     model.previousSelectedData = null;
+    model._captureInProgress = null;
     renderFrontBuffer();
-    model._captureInProgress = false;
   }

I did try it out with WebGPU + Chrome Canary, and I don't see anymore getSelection errors. However, the LineWidget cannot be interacted with once placed, and the spheres are not correctly colored. I haven't investigated that bit yet.

@finetjul
Copy link
Member Author

finetjul commented May 8, 2023

Thanks for your tweak, I added it to the PR.

In WebGPU, there are many rendering warnings which might be related:
image

In WebGL, it is working the same way as the existing example.

@floryst
Copy link
Collaborator

floryst commented May 16, 2023

LGTM after the RenderWindow conflict is resolved. Seems like there's more work to do with the WebGPU implementation.

@finetjul finetjul force-pushed the fix-widgetmanager-with-webgpu branch 2 times, most recently from 1967c53 to 465b9fd Compare August 28, 2023 06:11
@finetjul finetjul changed the title fix(webgpu): expose onWindowResizeEvent function to render window Web… fix(webgpu): Fix support of WebGPU rener backend Aug 28, 2023
@finetjul finetjul changed the title fix(webgpu): Fix support of WebGPU rener backend fix(webgpu): Fix support of WebGPU render backend Aug 28, 2023
@floryst floryst added this to the v29 milestone Aug 28, 2023
@floryst floryst changed the base branch from master to v29-merge October 26, 2023 15:55
Comment on lines 1281 to 1339
model._onSizeChanged = (_publicAPI, _model, newValue) =>
publicAPI.invokeWindowResizeEvent({
width: newValue[0],
height: newValue[1],
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@finetjul I was going to fix the conflicts and realized that model._onSizeChanged is not wired up anywhere. Is there a pathway for invoking the window resize event now that publicAPI.setSize() no longer directly invokes the event?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm partially reverting this such that setSize directly invokes the window resize event in both the OpenGL and WebGPU RenderWindows. The changes are all internal, so if you want to continue with the idea you had for _onSizeChanged, you can do that without a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_onSizeChanged was being called since this following commit
63787d5

TBH, I do not remember anything from this current PR though...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's good to know. I forgot about that change. We should document it somewhere.

finetjul and others added 4 commits October 26, 2023 12:18
With WebGPU, moving the mouse over was triggering many errors due to asynchronous captureBuffer
calls
Allow passing defaultViewAPI in vtkFullscreenRenderWindow
newInstance.

vtkFullscreenRenderWindow, vtkGenericRenderWindow and vtkViewProxy are
now exposing `getApiSpecificRenderWindow()` instead of
`getOpenGLRenderWindow()`.
`OpenGL/vtkBufferObject` and `OpenGL/vtkHardwareSelector` still expose
`getOpenGLRenderWindow()`.
@floryst floryst force-pushed the fix-widgetmanager-with-webgpu branch from 465b9fd to 1435f51 Compare October 26, 2023 16:30
@floryst floryst merged commit b41f27e into Kitware:v29-merge Oct 26, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants