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

VolumeMapper & OpenGLTexture: support updating image regions #3077

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

floryst
Copy link
Collaborator

@floryst floryst commented Jun 4, 2024

Context

When updating a 3D texture, the entire texture is uploaded to the GPU. If a volume is incrementally updated (e.g. slice-by-slice), then there is a lot of unchanged data being uploaded at every update.

This changeset lets the VolumeMapper specify image regions that have updated. Internally, this tells vtkOpenGLTexture to only update those regions via texSubImage3D.

This change can also be applied to other mappers that use the 3D textures, e.g. the ImageResliceMapper.

Results

The volume mapper now has the following methods: setUpdatedExtents(extents) and getUpdatedExtents(). This property defines which extents need to be updated.

const extent = [20, 49, 20, 49, 20, 49];
volumeMapper.setUpdatedExtents([...volumeMapper.getUpdatedExtents(), extent]);

This property acts as a "request" to update a set of extents. It is cleared after every successful render call.

Changes

  • Documentation and TypeScript definitions were updated to match those changes
  • vtkVolumeMapper and vtkImageResliceMapper: setUpdatedExtents() and getUpdatedExtents()
  • vtkOpenGLTexture: support updated extents in create3D* functions

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:
    • OS:
    • Browser:

@floryst
Copy link
Collaborator Author

floryst commented Jun 4, 2024

@sankhesh @finetjul it would be good to do a review of the API, since this is an initial draft at a working API.Some initial thoughts:

  • Maybe add a addRegionsToUpdate method
  • Should the regions be user-cleared or cleared after every render call

@floryst floryst force-pushed the streaming-textures branch 2 times, most recently from 2928f9f to eda0053 Compare June 4, 2024 21:22
Sources/Rendering/Core/VolumeMapper/index.d.ts Outdated Show resolved Hide resolved
Sources/Rendering/Core/VolumeMapper/index.d.ts Outdated Show resolved Hide resolved
Sources/Rendering/OpenGL/Texture/index.js Outdated Show resolved Hide resolved
Sources/Rendering/OpenGL/Texture/index.js Outdated Show resolved Hide resolved
Sources/Rendering/OpenGL/Texture/index.js Show resolved Hide resolved
Sources/Rendering/OpenGL/Texture/index.js Outdated Show resolved Hide resolved
@floryst
Copy link
Collaborator Author

floryst commented Jun 5, 2024

I've pushed a new commit:

  • rename "region" to "extent" and use the existing Extent type
  • minor perf improvements to the extent handling (not measured)

Copy link
Member

@finetjul finetjul left a comment

Choose a reason for hiding this comment

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

LGTM

@floryst floryst marked this pull request as ready for review June 11, 2024 17:06
@floryst
Copy link
Collaborator Author

floryst commented Jun 11, 2024

This is now ready for any final reviews @sankhesh. All commits will be squashed prior to merging.

A summary of the recent commits:

  • Add updatedExtents support for the vtkImageResliceMapper
  • Add a test for vtkOpenGLVolumeMapper and fix an updatedExtents-related bug in vtkOpenGLTexture

model.renderable.setUpdatedExtents([]);

// Build the image scalar texture
const dims = image.getDimensions();
model.openGLTexture.create3DFilterableFromDataArray(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider transitioning from positional arguments to object-based arguments. It would make it easier to track which arguments have default values and which ones need to be added. It wouldn't be a breaking change since this are not really public APIs that people use IMO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good idea to consider. Probably best to wrap up such a change with the next major version increment anyways to be safe from a public API standpoint (even if it's mostly used internally).

@finetjul finetjul requested a review from bruyeret August 5, 2024 06:53
Comment on lines +225 to +228
if (!model.openGLTexture) {
model.openGLTexture = vtkOpenGLTexture.newInstance();
model.openGLTexture.setOpenGLRenderWindow(model._openGLRenderWindow);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The code has changed and there will be conflicts here
Look at how shared textures are handled in the mappers to make sure you do not break the resource sharing or create memory leaks

);

model._paramCache = {
tex3dAllocated: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the point of tex3dAllocated, as it is always true?


// It's possible for the texture parameters to change while
// streaming, so check for such a change.
const rebuildEntireTexture =
Copy link
Contributor

Choose a reason for hiding this comment

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

You can create a newParamCache object and call DeepEqual between this newParamCache and model._paramCache. Then, if they are different, you just have to do model._paramCache = newParamCache;

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.

5 participants