feat(segmentation_user_layer): add individual segment color picker tool #636
Conversation
|
I still need to add the build time option to enable this feature |
This seems like a generally useful feature... can it be enabled by default? |
chrisj
force-pushed
the
cj_segment_list_color_selector_2
branch
from
05c36bd to
f804a1f
Compare
chrisj
force-pushed
the
cj_segment_list_color_selector_2
branch
from
f804a1f to
1519da4
Compare
|
@jbms can we take a look at this PR? I do think having this enabled by default for neuromancer-demo.appspot.com would be a good idea, but I'll leave that up to you. |
|
@seankmartin @jbms I think this is a fairly straightforward PR, can someone review it? |
|
I can try look in more detail, but this is a really nice feature, thanks! The only comments I have at a high level is that the build flag removing this feature from the frontend seems unfortunate since I think it would be widely helpful. I read the old PR for context. Could it instead be the case that the build flag just changes the entry point into the color changer? So with the flag for the separate widget you get what you have now. And without the flag you just get that alt-click on the ID element opens a color picker? Also would be great if the color select element had a tooltip, especially to incidate the right click to clear set color functionality |
|
@seankmartin Thanks for the suggestions, looking into it again to see if there was a reason I never implemented alt click as an alternative. |
chrisj
force-pushed
the
cj_segment_list_color_selector_2
branch
3 times, most recently
from
8ae854e to
c8aa823
Compare
|
I added alt+click to set and alt+shift+click to unset on top of the existing optional visible widget. I added some title attributes for both the widget and the row to show the click behavior. One issue with the non-visible widget, it is not always clear that the row has a stated color. I added a white 1px border. Which is clear in the screenshot below but basically hidden in the screenshot above with a white segment. One option would be to change the appearance of the visible toggle. Or we could forgo an indicator when the widget is hidden |
| const { selectedSegments } = displayState.segmentationGroupState.value; | ||
| selectedSegments.set(id, !selectedSegments.has(id)); | ||
| }); | ||
| const trackableRGB = new TrackableRGB(vec3.fromValues(0, 0, 0)); |
There was a problem hiding this comment.
I think it would be better to just add the <input type=color> element to the template and handle mouse events in the onMoueDown handler defined above.
The ColorWidget abstraction doesn't seem to help here.
| ); | ||
| children[template.colorWidgetIndex] as HTMLInputElement; | ||
| if (SEGMENT_LIST_COLOR_WIDGET_ENABLED) { | ||
| setColorWidgetColor( |
There was a problem hiding this comment.
Currently if SEGMENT_LIST_COLOR_WIDGET_ENABLED is false, then when you open the widget via alt+click the initial color does not match the current color. That could be fixed by calling setColorWidgetColor as part of the alt+click handler.
| import { makeFilterButton } from "#src/widget/filter_button.js"; | ||
| import { makeStarButton } from "#src/widget/star_button.js"; | ||
|
|
||
| declare const SEGMENT_LIST_COLOR_WIDGET: boolean | undefined; |
There was a problem hiding this comment.
This should be prefixed with NEUROGLANCER like all of the other build-time options to avoid (theoretical) name collisions, especially if neuroglancer is being embedded as a library.
chrisj
force-pushed
the
cj_segment_list_color_selector_2
branch
2 times, most recently
from
959c56b to
3187c72
Compare
|
@jbms Thanks for the review, I addressed those. One thing that was slightly awkward was needing to add template: SegmentWidgetTemplate to the onMousedown handler. |
src/widget/color.ts
Outdated
| Color extends vec3 | undefined = vec3, | ||
| > extends RefCounted { | ||
| element = document.createElement("input"); | ||
| static template() { |
There was a problem hiding this comment.
I think the changes in this file can be reverted now.
chrisj
force-pushed
the
cj_segment_list_color_selector_2
branch
from
3187c72 to
d0f8ebd
Compare
show individual color picker with alt+click on segment id, alt+shift+click to unset visible widget enabled with SEGMENT_LIST_COLOR_WIDGET option
chrisj
force-pushed
the
cj_segment_list_color_selector_2
branch
from
d0f8ebd to
09d5738
Compare
|
@jbms can we take another look at this one? |
5 participants
replaces #378
has a white border when widget has a set value, right click unsets the value