feat(vtkPiecewiseWidget): Support vertical movement of points #488

bnmajor · 2023-11-06T19:07:19Z

Vertical shift is allowed when holding control and left-click+dragging up/down. Shifting the points changes opacity.

Initial pass at fixing #188

netlify · 2023-11-06T19:07:24Z

✅ Deploy Preview for volview-dev ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`a998a42`
🔍 Latest deploy log	https://app.netlify.com/sites/volview-dev/deploys/65539cc2becfcd00081395a3
😎 Deploy Preview	https://deploy-preview-488--volview-dev.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

bnmajor · 2023-11-06T19:07:55Z

@aylward @floryst I would love some feedback to make sure this is on track/meets the expectations of the issue. Thanks!

aylward · 2023-11-06T22:18:35Z

Looks great...but I think the opacity value is inverted...

When I raise up the opacity curve...more becomes transparent...

And when I lower it, less becomes transparent...

This seems to be the opposite of what I'd expect and what the transfer function curve shows...

bnmajor · 2023-11-07T13:37:29Z

Looks great...but I think the opacity value is inverted...

I am sorry about that, the values are fixed now!

aylward

LGTM!

aylward · 2023-11-07T16:26:40Z

Actually - one request, if the opacity value is 0, let's not cause it to shift (that is, only apply the increase/decrease to non-zero opacity values - zero should always remain zero. That way, the user will be able to select/threshold the background and change the non-zero opacities assigned to the data up/down without the background becoming an issue.

aylward

Please see follow-on comment for one more change request - zero opacity values should always remain zero, i.e., should not be shifted up/down.

bnmajor · 2023-11-07T18:46:09Z

Actually - one request, if the opacity value is 0, let's not cause it to shift (that is, only apply the increase/decrease to non-zero opacity values - zero should always remain zero. That way, the user will be able to select/threshold the background and change the non-zero opacities assigned to the data up/down without the background becoming an issue.

Makes sense! I've pushed a change to handle this so only non-zero values are shifted.

aylward · 2023-11-07T20:39:53Z

Weird...without changing anything, the appearance of the prostatex MRI is different in your version:

Here is the original default rendering

Here is the default render provided by your version

This version perhaps has some kind of color saturation happening?

...Using your controls to slightly reduce global opacity removes the weird effect. So, perhaps your global opacity begins with a non-zero value or such?

aylward · 2023-11-07T20:45:35Z

Also - maybe you need to clip opacities to 1.0. If I raise the global opacity really high using an all white transfer function (white is the only color in the transfer function), black stuff still appears and some odd shading wonkiness happens...

aylward · 2023-11-07T20:49:02Z

src/utils/vtk-helpers.ts

+      // Non-zero values should be affected by shift
+      // but preset values of zero should not
+      const shifted = y - shiftAlpha;
+      const yVal = y || shifted ? shifted : -1;


Not certain || shifted is needed?

Should also clip range?

src/vtk/PiecewiseWidget/index.js

floryst · 2023-11-08T16:32:42Z

Sorry for ambiguity of the review. I meant to specify that ctrlKeyIsDown should be renamed to something more descriptive, since (1) it's not obvious what it does and (2) we don't want to imply that the ctrl key is the only key we can use for controlling the vertical/alpha opacity shift.

You can revert 9c8bc0d. As an aside, the naming of opacityShiftAlpha is mostly fine, though "alpha" and "opacity" generally refer to the same concept.

aylward · 2023-11-08T16:41:36Z

All issues I had found have been resolved. Works great!!

bnmajor · 2023-11-08T18:01:02Z

Sorry for ambiguity of the review. I meant to specify that ctrlKeyIsDown should be renamed to something more descriptive, since (1) it's not obvious what it does and (2) we don't want to imply that the ctrl key is the only key we can use for controlling the vertical/alpha opacity shift.

You can revert 9c8bc0d.

Ah, I'm sorry for the misunderstanding there. That makes sense.

As an aside, the naming of opacityShiftAlpha is mostly fine, though "alpha" and "opacity" generally refer to the same concept.

Agreed, the name is redundant. We're really just shifting the values so what about:

ctrlKeyIsDown -> shiftOpacityValues
opacityAlphaShift -> opacityValueShift

aylward · 2023-11-08T18:15:37Z

Still works great!

This is an outstanding extension - can create exceptionally nice views quickly.

One question - why CTRL and not SHIFT?

s

aylward · 2023-11-08T18:19:42Z

Oops - just found a bug...switch to certain transfer functions, and this happens...

I think the transfer function is correct - but the display of that transfer function over the histogram is wrong.

bnmajor · 2023-11-08T18:20:55Z

Still works great!

This is an outstanding extension - can create exceptionally nice views quickly.

One question - why CTRL and not SHIFT?

s

Great question! No reason, happy to change it to shift (or any key that might be preferred)

bnmajor · 2023-11-08T18:21:55Z

Oops - just found a bug...switch to certain transfer functions, and this happens...
I think the transfer function is correct - but the display of that transfer function over the histogram is wrong.

Funky... I'll check that out.

aylward · 2023-11-08T18:22:58Z

One more bug - seems like the transfer function being used by the LOD-reduced rendering is not being updated - only the final / high-quality transfer function is being adjusted.

You can see this by adjusting the opacity higher, and then moving the image - while moving it uses the un-adjusted transfer function with the LOD-reduced image. When you stop, it generates the high-quality image using the shifted opacities...

aylward · 2023-11-09T20:48:32Z

All looks good to me!

floryst · 2023-11-13T16:24:40Z

Agreed, the name is redundant. We're really just shifting the values so what about:

ctrlKeyIsDown -> shiftOpacityValues

opacityAlphaShift -> opacityValueShift

These names sound good to me. Let's go with these renames.

Vertical shift is allowed when holding control and left-click+dragging up/down. Shifting the points changes opacity.

The opacity should be increased when the points are shifted up and decreased when shifted down.

aylward · 2023-11-14T17:24:57Z

FYI: Still works great!

floryst · 2023-11-14T18:23:32Z

LGTM!

aylward assigned aylward and unassigned aylward Nov 7, 2023

aylward self-requested a review November 7, 2023 16:20

aylward approved these changes Nov 7, 2023

View reviewed changes

aylward reviewed Nov 7, 2023

View reviewed changes

floryst reviewed Nov 8, 2023

View reviewed changes

src/vtk/PiecewiseWidget/index.js Outdated Show resolved Hide resolved

bnmajor force-pushed the shift-opacity branch from 0b2600b to 9c8bc0d Compare November 8, 2023 16:23

bnmajor force-pushed the shift-opacity branch 2 times, most recently from d021471 to 2a706ab Compare November 8, 2023 18:07

bnmajor force-pushed the shift-opacity branch from 2a706ab to 63a32ab Compare November 8, 2023 18:22

bnmajor changed the title ~~feat(vtkPiecewiseWidget): Support horizontal movement of points~~ feat(vtkPiecewiseWidget): Support vertical movement of points Nov 8, 2023

bnmajor force-pushed the shift-opacity branch 2 times, most recently from 055df79 to d9c2986 Compare November 9, 2023 19:57

bnmajor added 3 commits November 14, 2023 11:12

feat(vtkPiecewiseWidget): Support vertical movement of points

4db3f1a

Vertical shift is allowed when holding control and left-click+dragging up/down. Shifting the points changes opacity.

fix(getShiftedOpacityFromPreset): fix inverted opacity value

f876878

The opacity should be increased when the points are shifted up and decreased when shifted down.

fix(shiftedOpacity): Do not shift opacity values of 0

858a25f

bnmajor force-pushed the shift-opacity branch from d9c2986 to dec6f82 Compare November 14, 2023 16:12

refactor(opacityShift): Rename variables for clarity

a998a42

bnmajor force-pushed the shift-opacity branch from dec6f82 to a998a42 Compare November 14, 2023 16:13

floryst added this pull request to the merge queue Nov 14, 2023

Merged via the queue into Kitware:main with commit 411e5a8 Nov 14, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vtkPiecewiseWidget): Support vertical movement of points #488

feat(vtkPiecewiseWidget): Support vertical movement of points #488

bnmajor commented Nov 6, 2023 •

edited

Loading

netlify bot commented Nov 6, 2023 •

edited

Loading

bnmajor commented Nov 6, 2023

aylward commented Nov 6, 2023 •

edited

Loading

bnmajor commented Nov 7, 2023

aylward left a comment

aylward commented Nov 7, 2023

aylward left a comment

bnmajor commented Nov 7, 2023

aylward commented Nov 7, 2023 •

edited

Loading

aylward commented Nov 7, 2023

aylward Nov 7, 2023

aylward Nov 7, 2023

floryst commented Nov 8, 2023

aylward commented Nov 8, 2023

bnmajor commented Nov 8, 2023

aylward commented Nov 8, 2023

aylward commented Nov 8, 2023

bnmajor commented Nov 8, 2023

bnmajor commented Nov 8, 2023

aylward commented Nov 8, 2023 •

edited

Loading

aylward commented Nov 9, 2023

floryst commented Nov 13, 2023

aylward commented Nov 14, 2023

floryst commented Nov 14, 2023

feat(vtkPiecewiseWidget): Support vertical movement of points #488

feat(vtkPiecewiseWidget): Support vertical movement of points #488

Conversation

bnmajor commented Nov 6, 2023 • edited Loading

netlify bot commented Nov 6, 2023 • edited Loading

✅ Deploy Preview for volview-dev ready!

bnmajor commented Nov 6, 2023

aylward commented Nov 6, 2023 • edited Loading

bnmajor commented Nov 7, 2023

aylward left a comment

Choose a reason for hiding this comment

aylward commented Nov 7, 2023

aylward left a comment

Choose a reason for hiding this comment

bnmajor commented Nov 7, 2023

aylward commented Nov 7, 2023 • edited Loading

aylward commented Nov 7, 2023

aylward Nov 7, 2023

Choose a reason for hiding this comment

aylward Nov 7, 2023

Choose a reason for hiding this comment

floryst commented Nov 8, 2023

aylward commented Nov 8, 2023

bnmajor commented Nov 8, 2023

aylward commented Nov 8, 2023

aylward commented Nov 8, 2023

bnmajor commented Nov 8, 2023

bnmajor commented Nov 8, 2023

aylward commented Nov 8, 2023 • edited Loading

aylward commented Nov 9, 2023

floryst commented Nov 13, 2023

aylward commented Nov 14, 2023

floryst commented Nov 14, 2023

bnmajor commented Nov 6, 2023 •

edited

Loading

netlify bot commented Nov 6, 2023 •

edited

Loading

aylward commented Nov 6, 2023 •

edited

Loading

aylward commented Nov 7, 2023 •

edited

Loading

aylward commented Nov 8, 2023 •

edited

Loading