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

Include segmentation layers in handling maximum number of renderable layers restricted by hardware #6211

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented May 17, 2022

This PR adds the existing segmentation layers of a dataset to the getLayersToRender function. This function handles the case that the number of activated layers exceeds the maximum available textures available to WebGL via driver/hardware/browser combination.
Previous to this pr, this was only done for color layers and one texture was always reserved for a segmentation layer. Now that there can be multiple segmentation layers, this led to rendering errors when the datasets color layer + segmentation layer count exceeds the hardware limit.
This pr changes the getLayersToRender so that it takes segmentation layer into account and at most returns a layer count equal to the hardware limit. This ensures that the code does not try to push all segmentation layers to GPU textures and therefore prevents rendering errors.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • I suggest testing this locally as it is easier to create datasets with a lot of layers for testing in that environment.
  • Create a dataset with (hardware maximum count - 2) color layers (just create copies of existing color layers). Then add a few segmentation layers to make sure you exceed the hardware limit in case all layers are active.
  • Check out the current version of the master and try to view the created test dataset. This should result in rendering errors similar to https://forum.image.sc/t/why-are-the-views-mono-color-without-data/64358.
  • Now check out this branch and reload the dataset. This version should no longer have rendering errors. The red error toast indicating too many active layers should still be there until you deactivate enough layers.

Issues:


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

Comment on lines -784 to -787
onlyColorLayers: boolean;
} = {
onlyColorLayers: false,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only code lines where the onlyColorLayers property was used are removed in this pr. Therefore I also remove this option to not have unused code lines

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Great generalization 👍 I found a small bug which I directly investigated. I created an annotation with 5 volume annotation layers, 3 segmentation layers and one color layer. When switching between the different volume annotation layers, I got invalid shaders for a brief amount of time (errors in console and red/green/blue viewports). I tracked it down to getLayersToRender which could yield duplicate entries for short time spans. I hot fixed it (see my latest commit) with a _.uniq call, but I'm not sure where this is coming from.

Possible reasons:

  • the listener which calls onDisableColorLayer is called too late so that it gets out of sync with the store briefly (i'm not really convinced)
  • the _.without calls don't really work, anymore, since the this.leastRecentlyVisibleColorLayers is not a array of strings, but of objects (which are not primitive --> the comparison doesn't work correctly?)

These are only wild guesses, but the second one seems plausible to me, since the error didn't exist before for color layers. Could you please have a short look at this? The uniq call per se is probably ok as a workaround, but if the actual reason is that _.without or something else doesn't work properly, there might be additional bugs..

@MichaelBuessemeyer MichaelBuessemeyer self-assigned this May 18, 2022
Copy link
Contributor Author

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Thanks for finding this bug 🐛 .

Sadly,

the listener which calls onDisableColorLayer is called too late so that it gets out of sync with the store briefly (i'm not really convinced)

seems to be exactly the case after I excluded your second guess. A solution I could come up with is to only trigger the shader recomputation after all calls to the onDisableLayer and onEnableLayer methods. But I am not totally convinced that this prevents this race condition. I can only tell, that the error no longer appears for me. @philippotto Could you please test whether you can still reproduce the error? Maybe it might still appear on a faster machine / non Mac laptop.

Comment on lines +668 to +672
const [sanitizedColorLayerNames, sanitizedSegmentationLayerNames] = _.partition(
names,
({ isSegmentationLayer }) => !isSegmentationLayer,
).map((layers) => layers.map(({ name }) => sanitizeName(name)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhhh 😲 , very nice 🚀

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome 👍 Thanks for fixing the race condition. Works great now 🚀

@MichaelBuessemeyer MichaelBuessemeyer merged commit 5bd20c4 into master May 24, 2022
@MichaelBuessemeyer MichaelBuessemeyer deleted the fix-layer-count-hareware-overflow-for-segmentation-layers branch May 24, 2022 13:48
hotzenklotz added a commit that referenced this pull request May 25, 2022
…e_of_conduct

* 'master' of github.com:scalableminds/webknossos:
  fix leowe slack notification (#6239)
  Volume annotation zarr streaming (#6203)
  Include segmentation layers in handling maximum number of renderable layers restricted by hardware (#6211)
  Allow restricting mags for new volume layer (#6229)
  Added documentation on Connectome Viewer (#6232)
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.

Better support datasets with many segmentation layers
2 participants