-
-
Notifications
You must be signed in to change notification settings - Fork 422
fix(OpenGL): release poly data mapper buffers when the view node is deleted #3618
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { it, expect } from 'vitest'; | ||
| import testUtils from 'vtk.js/Sources/Testing/testUtils'; | ||
| import { | ||
| createTrackedRenderView, | ||
| expectSameImageAfterRelease, | ||
| } from 'vtk.js/Sources/Testing/renderTestUtils'; | ||
|
|
||
| import vtkActor from 'vtk.js/Sources/Rendering/Core/Actor'; | ||
| import vtkConeSource from 'vtk.js/Sources/Filters/Sources/ConeSource'; | ||
| import vtkGlyph3DMapper from 'vtk.js/Sources/Rendering/Core/Glyph3DMapper'; | ||
| import vtkPlaneSource from 'vtk.js/Sources/Filters/Sources/PlaneSource'; | ||
|
|
||
| function createGlyphActor(gc) { | ||
| const planeSource = gc.registerResource(vtkPlaneSource.newInstance()); | ||
| const coneSource = gc.registerResource(vtkConeSource.newInstance()); | ||
| const mapper = gc.registerResource(vtkGlyph3DMapper.newInstance()); | ||
| mapper.setInputConnection(planeSource.getOutputPort(), 0); | ||
| mapper.setInputConnection(coneSource.getOutputPort(), 1); | ||
| const actor = gc.registerResource(vtkActor.newInstance()); | ||
| actor.setMapper(mapper); | ||
| return actor; | ||
| } | ||
|
|
||
| // Glyphs add per-instance matrix, normal, color and pick buffers. | ||
| it.skipIf(__VTK_TEST_NO_WEBGL__)( | ||
| 'frees the GPU objects of a glyph actor removed from a view', | ||
| () => { | ||
| const gc = testUtils.createGarbageCollector(); | ||
| const { tracker, renderer, renderWindow, emptySceneObjects } = | ||
| createTrackedRenderView(gc); | ||
|
|
||
| const actor = createGlyphActor(gc); | ||
| renderer.addActor(actor); | ||
| renderer.resetCamera(); | ||
| renderWindow.render(); | ||
| expect(tracker.count()).toBeGreaterThan(emptySceneObjects); | ||
|
|
||
| renderer.removeActor(actor); | ||
| renderWindow.render(); | ||
| expect(tracker.count()).toBe(emptySceneObjects); | ||
|
|
||
| gc.releaseResources(); | ||
| } | ||
| ); | ||
|
|
||
| it.skipIf(__VTK_TEST_NO_WEBGL__)( | ||
| 'rebuilds the same image after releaseGraphicsResources', | ||
| () => expectSameImageAfterRelease(createGlyphActor) | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| import { it, expect } from 'vitest'; | ||
| import testUtils from 'vtk.js/Sources/Testing/testUtils'; | ||
| import { | ||
| createTrackedRenderView, | ||
| expectSameImageAfterRelease, | ||
| } from 'vtk.js/Sources/Testing/renderTestUtils'; | ||
|
|
||
| import vtkActor from 'vtk.js/Sources/Rendering/Core/Actor'; | ||
| import vtkColorTransferFunction from 'vtk.js/Sources/Rendering/Core/ColorTransferFunction'; | ||
| import vtkConeSource from 'vtk.js/Sources/Filters/Sources/ConeSource'; | ||
| import vtkDataArray from 'vtk.js/Sources/Common/Core/DataArray'; | ||
| import vtkMapper from 'vtk.js/Sources/Rendering/Core/Mapper'; | ||
| import vtkOpenGLRenderWindow from 'vtk.js/Sources/Rendering/OpenGL/RenderWindow'; | ||
| import vtkRenderer from 'vtk.js/Sources/Rendering/Core/Renderer'; | ||
| import vtkRenderWindow from 'vtk.js/Sources/Rendering/Core/RenderWindow'; | ||
|
|
||
| function createConeActor(gc) { | ||
| const cone = gc.registerResource(vtkConeSource.newInstance()); | ||
| const mapper = gc.registerResource(vtkMapper.newInstance()); | ||
| mapper.setInputConnection(cone.getOutputPort()); | ||
| const actor = gc.registerResource(vtkActor.newInstance()); | ||
| actor.setMapper(mapper); | ||
| return actor; | ||
| } | ||
|
|
||
| // A lookup table makes the mapper own a color texture. | ||
| function createScalarColoredConeActor(gc) { | ||
| const cone = gc.registerResource(vtkConeSource.newInstance()); | ||
| cone.update(); | ||
| const polyData = cone.getOutputData(); | ||
| const pointCount = polyData.getPoints().getNumberOfPoints(); | ||
| polyData.getPointData().setScalars( | ||
| vtkDataArray.newInstance({ | ||
| name: 'scalars', | ||
| values: Float32Array.from({ length: pointCount }, (_, i) => i), | ||
| }) | ||
| ); | ||
|
|
||
| const lookupTable = gc.registerResource( | ||
| vtkColorTransferFunction.newInstance() | ||
| ); | ||
| lookupTable.addRGBPoint(0, 0, 0, 1); | ||
| lookupTable.addRGBPoint(pointCount, 1, 0, 0); | ||
|
|
||
| const mapper = gc.registerResource(vtkMapper.newInstance()); | ||
| mapper.setInputData(polyData); | ||
| mapper.setLookupTable(lookupTable); | ||
| mapper.setUseLookupTableScalarRange(true); | ||
| mapper.setInterpolateScalarsBeforeMapping(true); | ||
| const actor = gc.registerResource(vtkActor.newInstance()); | ||
| actor.setMapper(mapper); | ||
| return actor; | ||
| } | ||
|
|
||
| it.skipIf(__VTK_TEST_NO_WEBGL__)( | ||
| 'frees the GPU objects of actors removed from a view', | ||
| () => { | ||
| const gc = testUtils.createGarbageCollector(); | ||
| const { tracker, renderer, renderWindow, emptySceneObjects } = | ||
| createTrackedRenderView(gc); | ||
|
|
||
| const actors = [ | ||
| createConeActor(gc), | ||
| createConeActor(gc), | ||
| createScalarColoredConeActor(gc), | ||
| ]; | ||
| actors.forEach((actor) => renderer.addActor(actor)); | ||
| renderer.resetCamera(); | ||
| renderWindow.render(); | ||
| expect(tracker.count()).toBeGreaterThan(emptySceneObjects); | ||
|
|
||
| actors.forEach((actor) => renderer.removeActor(actor)); | ||
| renderWindow.render(); | ||
| expect(tracker.count()).toBe(emptySceneObjects); | ||
|
|
||
| gc.releaseResources(); | ||
| } | ||
| ); | ||
|
|
||
| it.skipIf(__VTK_TEST_NO_WEBGL__)( | ||
| 'frees only the closed view GPU objects on a shared context', | ||
| async () => { | ||
| const gc = testUtils.createGarbageCollector(); | ||
| const tracker = testUtils.trackWebGLObjects(); | ||
|
|
||
| const rootRenderWindow = gc.registerResource(vtkRenderWindow.newInstance()); | ||
| const rootView = gc.registerResource(vtkOpenGLRenderWindow.newInstance()); | ||
| rootRenderWindow.addView(rootView); | ||
| rootView.initialize(); | ||
|
|
||
| // The child render windows, their view nodes and their renderers are not | ||
| // gc-registered: the closing one is deleted explicitly below. | ||
| const addChildView = () => { | ||
| const childRenderWindow = vtkRenderWindow.newInstance(); | ||
| rootRenderWindow.addRenderWindow(childRenderWindow); | ||
| const childView = rootView.addMissingNode(childRenderWindow); | ||
| childRenderWindow.addView(childView); | ||
| childView.setContainer(testUtils.createRenderContainer(gc)); | ||
| childView.setSize(200, 200); | ||
|
|
||
| const renderer = vtkRenderer.newInstance(); | ||
| childRenderWindow.addRenderer(renderer); | ||
| renderer.addActor(createConeActor(gc)); | ||
| renderer.resetCamera(); | ||
|
|
||
| return { childRenderWindow, childView }; | ||
| }; | ||
|
|
||
| const closing = addChildView(); | ||
| const surviving = addChildView(); | ||
| rootRenderWindow.render(); | ||
|
|
||
| const bothViewsObjects = tracker.count(); | ||
| const survivingBefore = surviving.childView.captureNextImage(); | ||
| rootRenderWindow.render(); | ||
| expect(tracker.count()).toBe(bothViewsObjects); | ||
|
|
||
| rootRenderWindow.removeRenderWindow(closing.childRenderWindow); | ||
| closing.childRenderWindow.delete(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't this lead to double deletion if the garbage collector already tracks resources?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. The child render windows aren't gc-registered, and neither are their view nodes or renderers. The gc holds the root render window, the root view, the containers, and each child's cone source, mapper and actor, so
|
||
|
|
||
| const survivingAfter = surviving.childView.captureNextImage(); | ||
| rootRenderWindow.render(); | ||
|
|
||
| expect(tracker.count()).toBeLessThan(bothViewsObjects); | ||
| expect(await survivingAfter).toBe(await survivingBefore); | ||
|
|
||
| gc.releaseResources(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as before. |
||
| } | ||
| ); | ||
|
|
||
| // The scalar-colored actor makes the released color texture come back too. | ||
| it.skipIf(__VTK_TEST_NO_WEBGL__)( | ||
| 'rebuilds the same image after releaseGraphicsResources', | ||
| () => expectSameImageAfterRelease(createScalarColoredConeActor) | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { it, expect } from 'vitest'; | ||
| import testUtils from 'vtk.js/Sources/Testing/testUtils'; | ||
| import { createTrackedRenderView } from 'vtk.js/Sources/Testing/renderTestUtils'; | ||
|
|
||
| import vtkActor2D from 'vtk.js/Sources/Rendering/Core/Actor2D'; | ||
| import vtkCoordinate from 'vtk.js/Sources/Rendering/Core/Coordinate'; | ||
| import vtkLineSource from 'vtk.js/Sources/Filters/Sources/LineSource'; | ||
| import vtkMapper2D from 'vtk.js/Sources/Rendering/Core/Mapper2D'; | ||
|
|
||
| function createLineActor2D(gc) { | ||
| const line = gc.registerResource( | ||
| vtkLineSource.newInstance({ point1: [2, 2, 0], point2: [14, 14, 0] }) | ||
| ); | ||
| const coordinate = gc.registerResource(vtkCoordinate.newInstance()); | ||
| coordinate.setCoordinateSystemToWorld(); | ||
|
|
||
| const mapper = gc.registerResource(vtkMapper2D.newInstance()); | ||
| mapper.setInputConnection(line.getOutputPort()); | ||
| mapper.setTransformCoordinate(coordinate); | ||
| mapper.setScalarVisibility(false); | ||
|
|
||
| const actor = gc.registerResource(vtkActor2D.newInstance()); | ||
| actor.setMapper(mapper); | ||
| return actor; | ||
| } | ||
|
|
||
| it.skipIf(__VTK_TEST_NO_WEBGL__)( | ||
| 'frees the GPU objects of a 2D actor removed from a view', | ||
| () => { | ||
| const gc = testUtils.createGarbageCollector(); | ||
| const { tracker, renderer, renderWindow, emptySceneObjects } = | ||
| createTrackedRenderView(gc); | ||
|
|
||
| const actor = createLineActor2D(gc); | ||
| renderer.addActor2D(actor); | ||
| renderer.resetCamera(); | ||
| renderWindow.render(); | ||
| expect(tracker.count()).toBeGreaterThan(emptySceneObjects); | ||
|
|
||
| renderer.removeActor2D(actor); | ||
| renderWindow.render(); | ||
| expect(tracker.count()).toBe(emptySceneObjects); | ||
|
|
||
| gc.releaseResources(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is inside an async call, this may never get called if the test fails early leaking resources. Recommend enclosing inside an
afterEachto ensure to ensure cleanup.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for taking a look!
Agreed, a trailing
gc.releaseResources()is skipped whenever the test doesn't reach the end.Put the cleanup in
createGarbageCollectorinstead ofafterEach:expectSameImageAfterReleasecreates its owngc, so anafterEachin the test file would have nothing to release. Registering it increateGarbageCollectorcovers all three spots and matchescreateWebGPUTestDevicein the same file. Releasing twice is a no-op.