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

feat(ImageSlice): add forceOpacity and forceTransparent #3128

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

PaulHax
Copy link
Collaborator

@PaulHax PaulHax commented Sep 27, 2024

To maintain consistent render effect with stacked image slices no matter the opacity of the slices.

Context

When the prop's transparency crosses from .999 to 1, it gets slotted in the Opaque render pass and can change its render order: https://github.com/Kitware/vtk-js/blob/master/Sources/Rendering/Core/ImageSlice/index.js#L24-L26

When multiple slices are layered changing the tranparency can inavertently change how they are blended. Would be nice to keep a slice in a consistent render pass despite the opacity. Then can avoid this workaround: https://github.com/Kitware/VolView/pull/654/files#diff-3b32a6ae4684ef94faf56b59ea525d8c8adccc792d96cb56613eec2aedf6faf6R20

Results

From an added test: before with 2 actors where one is translucent

  actorBelow.getProperty().setOpacity(0.9);
  actorAbove.getProperty().setOpacity(1);

image

After with adding actorAbove.setForceTranslucent(true)
image

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

To maintain consistent render effect with stacked image slices
no matter the opacity of the slices.

closes Kitware#3126
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 added this pull request to the merge queue Sep 30, 2024
Merged via the queue into Kitware:master with commit 49bf5c8 Sep 30, 2024
3 checks passed
Copy link

🎉 This PR is included in version 32.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sedghi
Copy link
Contributor

sedghi commented Oct 7, 2024

Is this a workaround for a bug in rendering? I'm trying to understand when to force and when not to force?

@finetjul
Copy link
Member

finetjul commented Oct 7, 2024

You may want to forceOpaque when you want to render something translucent but writing in the depth buffer.
You may want to forceTransparent when you want to render something opaque without writing in the depth buffer.

@sedghi
Copy link
Contributor

sedghi commented Oct 7, 2024

Thanks @finetjul

I guess the default (without setting these force properties) is to render something translucent without a depth buffer and render something opaque with writing to the depth buffer?

What are the main reasons that one might or might not want to write to the depth buffer?

@finetjul
Copy link
Member

finetjul commented Oct 7, 2024

I guess the default (without setting these force properties) is to render something translucent without a depth buffer and render something opaque with writing to the depth buffer?

Yes, this is the default.

What are the main reasons that one might or might not want to write to the depth buffer?

Want to write in depth buffer: if you don't want other actors to render behind the actor
Don't want to write in depth buffer: if you want other actors to render behind/in front of the actor

I do not have a special use case in mind (besides Paul's), but I remember needing those in VTK C++. That's why Paul just "brought" those "forceXXX" functions from VTK-C++ to VTK.js

@sedghi
Copy link
Contributor

sedghi commented Oct 7, 2024

Amazing, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Automated label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants