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

Custom segment colors with cuckoo hashing #6417

Merged
merged 83 commits into from
Sep 15, 2022

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Aug 22, 2022

URL of deployed dev instance (used for testing):

Todo:

  • implement and adapt cuckoo table for storing colors
  • build UI to edit colors
  • use integer textures to store colors for 32 bit segment ids
  • refactor initial proof-of-concept
  • use custom colors for meshes, too
  • make compatible with multiple segmentation layers
  • make compatible with JSON mappings
    • unify with custom colors in JSON mappings
  • fix conceptual bug in merger mode (random colors vs unique ids)
  • fix bugs
    • diffing segment map should work against correct map
    • hiding volume layer produces invalid shader
    • fix that segment colors won't work when the tracing is loaded with no segmentation visible initially
  • fix CI
  • doublecheck compatibility with texture count constraints
  • debounce update_segments actions or compact the update actions
  • test thoroughly

Steps to test:

  • open an annotation with a segmentation layer
    • click a segment
    • go to the segments tab and rightclick a segment to change its color
    • also load an ad-hoc mesh for that segment
    • change the color again --> the color in the viewport, in the segments tab and in the 3D viewport for the mesh should change
    • save & reload
    • the set color should still be there
  • add a second volume layer
    • brush and change the segments color
    • switch between the volume layers and ensure that the colors are not mixed up somehow
  • test the merger mode a bit
  • open a dataset with a segmentation JSON mapping that has custom colors (e.g., ROI2017_wkw with mini-mapping-with-colors.json)
    • activate that mapping and see that the custom colors were used

Issues:


(Please delete unneeded items, merge only when none are left open)

  • Ready for review

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

First part of my review, I'm missing frontend/javascripts/oxalis/shaders/segmentation.glsl.ts and the actual frontend/javascripts/oxalis/model/bucket_data_handling/cuckoo_table.ts, also testing. Will continue tomorrow :)

global.performance = {
now: () => Date.now(),
};
mock("libs/window", {
Copy link
Member

Choose a reason for hiding this comment

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

What about the window "polyfill" in frontend/javascripts/libs/window.ts - do we need this mock?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch 👍 after I changed the requestAnimationFrame mock (from an immediate resolve to a setTimeout with 10ms), this worked. without a delay, there was an infinite recursion. with 100 ms some tests failed. so, I hope that the 10ms are not flakey (I did several runs and they all succeeded with 10). let's watch out in the next weeks for this :)

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Very cool! Another longstanding issue, finally resolved 🎉

My observations from testing:

  • The mesh loading is flickering weirdly. Maybe this is related to your changes in the mesh creation code.
  • If I activate mini-mapping-with-colors in the ROI2017_wkw dataset, the mapped segments have the correct color. However, using the "Reset Segment Color" functionality from the segment list, seems to behave weirdly - the segment and icon colors no longer match. Also when setting another color, afterwards (which works) and then resetting the color again, the segment color is not reset (only the icon color).
  • There's another method to specify mapping colors, using the frontend API. For example, on the test-agglomerate-file dataset, use webknossos.apiReady(3).then(async (api) => {api.data.setMapping(api.data.getVolumeTracingLayerName(), {1: 2}, {colors: [0.25, 0.5]})}). It seems to behave differently between the current master and this branch, the mapped colors (and the unmapped ones) are different. I'm not sure whether we still want to support this feature 🤷
  • The color picker should be initialized with the current color (as is the case when changing the tree color)

@daniel-wer
Copy link
Member

The changes up until a38fdeb look good to me. I think if you take care of the remaining ToDos and do a final test round, this should be ready to go, although I cannot finally approve it while I'm out of office.

@philippotto
Copy link
Member Author

Very cool! Another longstanding issue, finally resolved 🎉

My observations from testing:

Thanks for your thorough testing 🙏

  • The mesh loading is flickering weirdly. Maybe this is related to your changes in the mesh creation code.

This is fixed now. I had optimized the mesh chunks to share one material, however, the chunks are animated individually when loading which is why I had to remove the optimization again.

  • If I activate mini-mapping-with-colors in the ROI2017_wkw dataset, the mapped segments have the correct color. However, using the "Reset Segment Color" functionality from the segment list, seems to behave weirdly - the segment and icon colors no longer match. Also when setting another color, afterwards (which works) and then resetting the color again, the segment color is not reset (only the icon color).

This turned out to be two errors: (a) the cuckoo table didn't overwrite keys correctly (I added a regression test for this) and (b) there were some spots which used the old customColor object. Should work now.

  • There's another method to specify mapping colors, using the frontend API. For example, on the test-agglomerate-file dataset, use webknossos.apiReady(3).then(async (api) => {api.data.setMapping(api.data.getVolumeTracingLayerName(), {1: 2}, {colors: [0.25, 0.5]})}). It seems to behave differently between the current master and this branch, the mapped colors (and the unmapped ones) are different. I'm not sure whether we still want to support this feature shrug

I found this method to be quite buggy on master, too (the color hue didn't really have an impact). However, on this branch, the custom colors weren't properly used, either. I added code for this, but also added some analytics code so that we can check later whether this functionality is really used. The different types of how a mapping can be defined (mapping object vs equivalence classes) was once again a bit annoying, which is why I'd hope that we can remove it soon.

@philippotto philippotto merged commit d176b59 into segment-color Sep 15, 2022
@philippotto philippotto deleted the segment-color-cuckoo branch September 15, 2022 12:19
philippotto added a commit that referenced this pull request Sep 21, 2022
* [WIP] allow custom colors for volume annotation segments

* pretty

* include color in segment update actions

* fix compile errors

* backend tests

* set color to none if no color was included in the update action

* Custom segment colors with cuckoo hashing (#6417)

* resurrect cuckoo table implementation

* adapt CuckooTable to entries consisting of (number, vec3) and finish proof-of-concept for writing and reading that hash table for segment colors

* fall back to old color generation if custom color could not be looked up

* add UI to select segment color

* correctly save segment color to back-end

* hook up custom segment colors from store with texture

* improve performance of cuckoo updates

* fix isDisabled prop for interpolation button

* convert usage of vec4 color to vec3

* remove type property for uniforms since it's not needed anymore since three r76

* adapt tests to new cuckootable interface

* clean up a bit

* fix cuckoo tests by using broader value range for tests and using uint32array instead of float

* try to get uint32 texture working (int texture only works when defined as byte, but that will clip values to 8 bit)

* dirty proof of concept for uint32 texture

* restore custom color rendering

* fix tests

* DRY mocks in tests

* more clean up

* implement unset for cuckoo table to allow resetting a segment color

* extend tests a bit

* don't hardcode values for cuckoo shader code

* fix linting

* also show segment patterns for custom segment colors

* show correct colors (even when mapped) in segment list

* also use correct color for mesh and synchronize the mesh with the segment color

* fix linting

* clean up some typing

* make custom segment colors compatible with multiple segmentation layers

* fix linting

* fix segment color icon for non-tracing layers

* don't allow changing colors for segments of non-tracing segmentation layer

* remove unused code for low-level texture setup

* remove logging from cuckoo table implementation

* clean up cuckoo_table.spec.ts

* fix React does not recognize the  prop on a DOM element. warning when opening context menu

* don't rely on random IDs in merger mode

* dont use the term color in the merger mode implementation as it is incorrect and confusing

* remove shuffleColorOfCurrentTree functionality for merger mode (let's wait for user feedback to see whether this is really needed)

* also reset stored representative when deleting last node of tree in merger mode

* clean up some mapping related code

* allow changing segment colors for non-tracing segmentation layers (necessary for json mappings); the colors won't be saved, though

* use custom segment color feature when loading a mapping which defines hue values

* clean up typing for withMappingActivationConfirmation

* remove old custom color implementation for JSON mappings

* remove attempt to integrate ACE editor

* move the cuckoo table maintenance into the LayerRenderingManager to avoid redundant updates and ensure diffing with the correct segments

* fix linting

* fix that shader won't render when all volume layers are invisible (but at least one exists)

* don't fill int texture with 255 by default

* revert temporary changes in test.sh

* skip shader tests which try to parse glsl 3000 syntax (and fail)

* fix some tests by having isosurface-related sagas wait for the scene controller to be initialized (which doesn't happen in some tests); therefore fixing null access to the scene controller

* skip mapping-related texture requirement test

* fix that seeds were not initialized correctly when no segmentation is visible initially

* explain why some glsl tests are skipped

* remove isMappingSupported distinction and incorporate custom colors texture into rendering logic

* fix skipped test in rendering logic spec

* implement compaction for update segment UpdateActions

* explain texture format etc better

* clean up test

* make getSegmentsForLayer non-nullable

* fix shader tests by avoiding hex uint notation

* explain isInt workaround

* remove one window mock; enforce correct type of other window mock; remove lots of @ts errors

* explain disabled logic

* fix linting

* fix linting

* initialize color picker properly even when a custom segment color was not defined yet

* don't share material for chunks of mesh, since the opacity has to be controlled individually for the loading animation

* fix linting

* fix crash when anchor position of segment is null

* try to debug buggy restore

* fix bug with repeated sets that caused a key to be appear multiple times; also add regression test

* fix that mapping.mappingColors were still used in toolbar and segments tab; clean up

* fix linting

* avoid that all segments in segment list rerender when mouse is moved

* make custom colors work when using setMapping via frontend API

* add analytics for custom colors in setMapping

* memoize diffing of segments; refactor memoization of diffing trees

* add docs for segment list

* remove obsolete type parameter for uniform

Co-authored-by: Philipp Otto <[email protected]>
Co-authored-by: Philipp Otto <[email protected]>
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.

3 participants