Basic/advanced mode transfer function editor #294

frasercl · 2024-07-11T21:15:27Z

Review time: medium-large (~45min)

Resolves #80 and potentially #131: implements the final piece of @lynwilhelm's channel settings editor redesign by adding a "basic" mode to the transfer function editor, in which only a min and max of a ramp may be set, and hiding the current control points editor as "advanced" mode.

Adds two new fields to ViewerState: ramp: [number, number] for the min and max of the ramp set by "basic" mode, and useControlPoints: boolean to indicate whether to set the transfer function based on ramp or the existing controlPoints field.
Adds an "advanced" checkbox to the transfer function editor, which controls useControlPoints.
In TfEditor's new "basic" mode:
- Control points are replaced by two purple sliders for the min and max
- The horizontal axis labels get pushed down out of the way of the min slider
- Two additional numeric inputs appear for the min and max
When a time or region change causes the transfer function to be remapped, the viewer will also remap whichever state is not currently being used to set the transfer function (controlPoints in "basic" mode, ramp in "advanced" mode) so that it doesn't get out of sync.

Screenshots

…ditor

…dvanced mode

…ditor

toloudis · 2024-07-12T20:42:16Z

src/aics-image-viewer/shared/utils/controlPointsToLut.ts

+  }
+
+  // TODO: this creates a redundant Uint8Array and algorithmically fills it twice. Can we avoid this?
+  const remapLut = new Lut().createFromControlPoints(controlPoints);


I think you can just call remapControlPoints which is an exported function out of Lut.ts in the volume-viewer, without constructing this temporary remapLut. We could export it at the top level of the volume-viewer package, or make it some kind of class method on Lut...

Not currently exported from volume-viewer in a form we can use, but will be once allen-cell-animated/vole-core#227 merges.

src/aics-image-viewer/shared/utils/controlPointsToLut.ts

toloudis · 2024-07-12T21:00:28Z

src/aics-image-viewer/components/App/ChannelUpdater.tsx

+  const { controlPoints, ramp, useControlPoints } = channelState;
+  useImageEffect(
+    (currentImage) => {
+      if (useControlPoints && controlPoints.length < 2) {


what is this if condition checking for? is it for before any channel data has arrived, or something like that? I see it was there in the previous implementation too

Looks like this triggers only when the user drags a control point all the way to the left, then all the way to the right, thereby deleting all but one point. The effect is that the transfer function is left in its last valid state rather than getting zeroed out when there's only one control point. Does this behavior make sense?

I guess? It kinda sounds like something that could be handled inside the tfeditor but at least I get the scenario where it could happen.

toloudis · 2024-07-12T21:06:07Z

src/aics-image-viewer/components/App/index.tsx

@@ -128,6 +134,8 @@ const DEFAULT_CHANNEL_STATE: ChannelState = {
  isovalue: 128,
  opacity: 1.0,
  color: [226, 205, 179] as ColorArray,
+  useControlPoints: false,
+  ramp: [0, TFEDITOR_MAX_BIN],


Not sure if this would make anything cleaner but maybe we could have the control points default to the equivalent of [[0, 0], [TFEDITOR_MAX_BIN, 1]]

Also, rather than a bin index, we probably should think of the "x" domain of the transfer function (aka LUT) as intensities. It's a function of input intensity to output intensity.

If that incurs a crazy amount of changes then we should save it for a different PR and possibly wait for the full dynamic range modifications. ( also related to putting intensity numbers along the x axis in the UI.)

toloudis · 2024-07-12T21:16:32Z

src/aics-image-viewer/components/ChannelsWidgetRow/index.tsx

-  const onOpacityChange = ([newValue]: number[]): void => _onOpacityChange(newValue / ISOSURFACE_OPACITY_SLIDER_MAX);
+  const onIsovalueChange = ([newValue]: number[]): void => changeSettingForThisChannel("isovalue", newValue);
+  const onOpacityChange = ([newValue]: number[]): void =>
+    changeSettingForThisChannel("opacity", newValue / ISOSURFACE_OPACITY_SLIDER_MAX);


should this replace all the other usages of props.changeChannelSetting in this component, like the ones above for volumeEnabled and isosurfaceEnabled?

toloudis · 2024-07-12T21:24:09Z

src/aics-image-viewer/components/TfEditor/index.tsx

+// |   |
+//  \ /
+//   *
+// width: 0.65 * height; height of rectangle: 0.6 * height; height of triangle: 0.4 * height


❤️ love this -- but semi-wondering if a svg does any better here.

This is an svg! The context works like a canvas, but D3 turns it into an svg data string.

src/aics-image-viewer/components/TfEditor/index.tsx

toloudis · 2024-07-12T21:31:14Z

src/aics-image-viewer/components/TfEditor/index.tsx

+  const dragRampSlider = (handle: TfEditorRampSliderHandle, x: number): void => {
+    if (handle === TfEditorRampSliderHandle.Min) {
+      const max = rampRef.current[1];
+      setRamp([Math.min(x, max), max]);


can you drag the min handle past max and vice versa?

nope - this line ensures that each slider stops at the other slider's position rather than dragging past it.

toloudis · 2024-07-12T21:34:43Z

src/aics-image-viewer/components/TfEditor/index.tsx

-      setSelectedPointIdx(draggedPointIdxRef.current);
-    } else if (idxRight > draggedIdx + 1) {
-      newControlPoints.splice(draggedIdx + 1, idxRight - draggedIdx - 1);
+    if (typeof draggedPointIdxRef.current === "number") {


what is the other type that's not a "number"? should you just check props.useControlPoints here instead? or maybe draggedPointIdxRef.current as number vs draggedPointIdxRef.current as minmaxsliderenumthingy in the lines below - for readability

I agree that props.useControlPoints would make more semantic sense here. But it also wouldn't cause TypeScript to narrow this type for the following lines, where draggedPointIdxRef.current is passed either to a function that takes a number or to one that takes a TfEditorRampSliderHandle.

frasercl · 2024-07-15T21:29:10Z

The above 3 commits additionally resolve #281. The fix is quick, and that area of code is changed enough by this PR that fixing on main and merging would have been way more work.

ShrimpCryptid · 2024-07-16T23:42:08Z

src/aics-image-viewer/shared/utils/controlPointsToLut.ts

+  if (controlPoints.length < 3) {
+    return [0, TFEDITOR_MAX_BIN];
+  }
+  return [controlPoints[1].x, controlPoints[2].x];
+}


What does it mean if there are only two (or fewer) control points? Is one control point always at (0, 0.0), and the other at (255, 1.0)?

Took this callout as an opportunity to make this function more robust (I knew I wasn't quite dotting all my i's the first time around) but yeah: all the lut generator functions in volume-viewer add those two points at the ends.

ShrimpCryptid

Left a few questions but otherwise LGTM!

ShrimpCryptid · 2024-07-17T00:30:33Z

src/aics-image-viewer/components/TfEditor/index.tsx

+        controlPointsRef.current = props.controlPoints;
+      }
+
+      if (typeof draggedPointIdxRef.current !== "string") {


When would this be a string value?

This can either be a number, representing a control point, in "advanced" mode; or either "min" or "max" representing a slider handle in basic mode. See my response to @toloudis, above:

...props.useControlPoints would make more semantic sense here. But it also wouldn't cause TypeScript to narrow this type for the following lines...

Since this has now tripped up two people, it might need a comment

toloudis · 2024-07-17T21:12:48Z

The above 3 commits additionally resolve #281. The fix is quick, and that area of code is changed enough by this PR that fixing on main and merging would have been way more work.

For future, the way to do it with a pr is with a branch off of this branch, and a PR that uses this branch as its base. Then you just have to be careful not to merge the new pr before the older one.

frasercl · 2024-07-17T23:44:57Z

For future, the way to do it with a pr is with a branch off of this branch, and a PR that uses this branch as its base. Then you just have to be careful not to merge the new pr before the older one.

This occurred to me mere moments after pushing the commits onto this branch 🙃

frasercl added 30 commits June 21, 2024 12:05

create symbol for handle (first try)

c81d933

Merge branch 'refactor/functionize-tf-editor' into feature/basic-tf-e…

d495e68

…ditor

rename: handleSymbol -> sliderHandleSymbol

cd448aa

ChannelsWidgetRow just receives the full channel state object

c28ad49

channel state type receives new "basic transfer function" fields

8eff6f1

add basic/advanced tf conversion utils; use them to update tf

158c2bf

actually I'd rather represent the ramp as a pair rather than two fields

54f95bd

TfEditor just gets a generic channel setting updater

ffd5a2d

set "advanced mode" state from checkbox; show some elements only in a…

c731726

…dvanced mode

set up new colors in basic tf editor

e3dc19a

add basic tf editor sliders

768877e

sliders get the right cursor

804cf2c

add TODO for to-doing later

65d740b

basic mode handles are draggable

6068e2e

transfer function display and presets work in basic mode

2e118b9

forgot to remove a console.log

b7a5881

colorize isn't enabled in basic mode

561f3b0

Merge branch 'refactor/functionize-tf-editor' into feature/basic-tf-e…

6d44489

…ditor

gradient updates properly in basic mode

2e18325

fix upper slider handle running off top of svg

4f846f7

add a comment

792b013

make cursor type more stable when dragging in basic mode

4ff6cb2

Merge branch 'main' into feature/basic-tf-editor

95505a3

fix type errors in url_utils tests

d8cd5fe

fix: can't import from .tsx file in tests

5254053

automatically remapped control points get copied to correct state

55b598e

remap unused state (basic/advanced) in tf editor

6cd6e13

remove a console.log

5dada32

colorize slider is retained in basic mode

861083d

first pass: add numeric inputs to basic mode

d215f1c

frasercl added 2 commits July 11, 2024 13:59

use ant's InputNumber instead

1476e70

add a comment

db55b42

frasercl requested a review from a team as a code owner July 11, 2024 21:15

frasercl requested review from toloudis and ShrimpCryptid and removed request for a team July 11, 2024 21:15

Merge branch 'main' into feature/basic-tf-editor

d8b0edb

toloudis reviewed Jul 12, 2024

View reviewed changes

frasercl mentioned this pull request Jul 12, 2024

Two lut enhancements allen-cell-animated/vole-core#227

Merged

frasercl added 5 commits July 15, 2024 11:15

fix: rampToControlPoints accounts for ramp values outside 0-255 range

97256f9

replace a number-plus-string with a format string

e7d55a8

remove ramp ref

24fa80e

reorder stuff

2787072

fix dragging control point undoing remapping for other points

f4e4f0c

toloudis approved these changes Jul 15, 2024

View reviewed changes

ShrimpCryptid reviewed Jul 16, 2024

View reviewed changes

ShrimpCryptid approved these changes Jul 17, 2024

View reviewed changes

frasercl added 4 commits July 17, 2024 16:50

add comment

4501dac

make controlPointsToRamp a bit more robust

7f36ea2

one more small change for robustness

d59e7a7

Merge branch 'main' into feature/basic-tf-editor

1ce4d69

frasercl merged commit ca27665 into main Jul 18, 2024
3 checks passed

frasercl deleted the feature/basic-tf-editor branch July 18, 2024 00:13

This was referenced Jul 18, 2024

Moving a control point causes other control point to jump #281

Closed

idea for redoing the transfer function editor #131

Closed

frasercl mentioned this pull request Jul 26, 2024

Use control point remap function #300

Merged

frasercl mentioned this pull request Aug 20, 2024

Suggestion: add color ramp options #315

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic/advanced mode transfer function editor #294

Basic/advanced mode transfer function editor #294

frasercl commented Jul 11, 2024 •

edited

Loading

toloudis Jul 12, 2024

frasercl Jul 15, 2024 •

edited

Loading

toloudis Jul 12, 2024

frasercl Jul 15, 2024

toloudis Jul 15, 2024

toloudis Jul 12, 2024

toloudis Jul 12, 2024

toloudis Jul 12, 2024 •

edited by ShrimpCryptid

Loading

toloudis Jul 12, 2024

frasercl Jul 12, 2024 •

edited

Loading

toloudis Jul 12, 2024

frasercl Jul 12, 2024

toloudis Jul 12, 2024

frasercl Jul 15, 2024 •

edited

Loading

frasercl commented Jul 15, 2024

ShrimpCryptid Jul 16, 2024

frasercl Jul 18, 2024

ShrimpCryptid left a comment

ShrimpCryptid Jul 17, 2024

frasercl Jul 17, 2024

toloudis commented Jul 17, 2024

frasercl commented Jul 17, 2024

Basic/advanced mode transfer function editor #294

Basic/advanced mode transfer function editor #294

Conversation

frasercl commented Jul 11, 2024 • edited Loading

Screenshots

Choose a reason for hiding this comment

frasercl Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toloudis Jul 12, 2024 • edited by ShrimpCryptid Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frasercl Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frasercl Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

frasercl commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShrimpCryptid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toloudis commented Jul 17, 2024

frasercl commented Jul 17, 2024

frasercl commented Jul 11, 2024 •

edited

Loading

frasercl Jul 15, 2024 •

edited

Loading

toloudis Jul 12, 2024 •

edited by ShrimpCryptid

Loading

frasercl Jul 12, 2024 •

edited

Loading

frasercl Jul 15, 2024 •

edited

Loading