webknossos icon indicating copy to clipboard operation
webknossos copied to clipboard

Custom segment colors with cuckoo hashing

Open philippotto opened this issue 3 years ago • 4 comments

URL of deployed dev instance (used for testing):

  • https://segmentcolorcuckoo.webknossos.xyz/

Todo:

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

  • #6306

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

  • [ ] Updated (unreleased) changelog
  • [ ] Updated (unreleased) migration guide if applicable
  • [ ] Updated documentation if applicable
  • [ ] Adapted wk-connect if datastore API changes
  • [ ] Adapted wk-libs python client if relevant API parts change
  • [ ] Needs datastore update after deployment
  • [x] Ready for review

philippotto avatar Aug 22 '22 12:08 philippotto

@daniel-wer This PR is very close to being ready for review :) One last thing which is missing is adapting the code which ensures the correct texture count is available and respected for the GPU. The old code also checked whether JSON mappings are supported (by checking whether there are 3 textures to spare). If this condition wasn't met, the mapping feature was effectively disabled (however, I'm not sure how well this was integrated in the UI, since I was still able to select a mapping when I provoked this). With the new custom color implementation, the mapping support only needs 2 textures and piggybacks on new other custom color texture (which I would like to have always available --> would always block 1 texture if a segmentation layer is available). Now, I'm wondering whether the whole "are mappings supported" distinction is still necessary. WebGL 2 demands that at least 32 textures may exist and that the fragment shader may access at least 16 (source). So, if we block 3 textures for mappings and custom colors (if a segmentation layer exists), there are still 13 textures available. With a typical texture count of 3 per layer this gives the user at least 4 layers which can be rendered at the same time. If the limit is exceeded, layers will be automatically hidden (and the user is told so). I think this would be fair and means that we could remove the "are mappings supported" distinction. Do you agree? Not sure if there were some other reasons to build it like it currently is. Happy to jump on a call :)

philippotto avatar Aug 26 '22 10:08 philippotto

I think this would be fair and means that we could remove the "are mappings supported" distinction. Do you agree? Not sure if there were some other reasons to build it like it currently is. Happy to jump on a call :)

Thanks for explaining the issue in such detail! I agree that it is likely more user friendly to be able to show four "fully functional" layers than five layers without mapping support. I think the only advantage of having the distinction would be if there was no segmentation layer. In that case, users could maybe render one more layer, because the mappings are not needed. However, if it complicates the code a lot I'd be ok with removing the distinction. We'll notice if users complain that not enough layers can be rendered.

daniel-wer avatar Aug 29 '22 05:08 daniel-wer

This PR is very close to being ready for review :)

Let me know once you think I should start reviewing :)

daniel-wer avatar Aug 29 '22 05:08 daniel-wer

I think the only advantage of having the distinction would be if there was no segmentation layer. In that case, users could maybe render one more layer, because the mappings are not needed. However, if it complicates the code a lot I'd be ok with removing the distinction. We'll notice if users complain that not enough layers can be rendered.

Thanks for your opinion :+1: I changed the code to remove the distinction, but I kept the "does a segmentation layer exist?" handling so that the three special textures are not blocked in that case.

I think you can start reviewing now :)

philippotto avatar Aug 29 '22 08:08 philippotto

The changes up until a38fdeb0305cd6fddcefb7990f2dbc80543584fe 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.

daniel-wer avatar Sep 07 '22 11:09 daniel-wer

Very cool! Another longstanding issue, finally resolved :tada:

My observations from testing:

Thanks for your thorough testing :pray:

  • [x] 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.

  • [x] 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.

  • [x] 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 avatar Sep 15 '22 12:09 philippotto