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

Fix initial min/max values of a histogram of a color layer #5853

Merged
merged 7 commits into from
Nov 22, 2021

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Nov 15, 2021

It is possible to configure only the min and max for a color layer in the view configurations of a dataset. This causes that the backend to send the default values for the intensityRange for the color layer.
IIn case the min is higher than the sent intensity minimum or the max is lower than the sent intensity maximum the frontend got into an inconsistent state where the intensityRange is out of bounds considering the min and max.

A short solution to this is to force the intensityRange to be within the range given by min and max.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Go to the settings of a dataset and then to the "View Configurations"-tab.
  • There choose a (e.g. uint8) color layer and set min to 100 and max to 150.
  • Then make the dataset publically available under the "Sharing & Permissions"-tab, copy the sharing link and save the settings.
  • Then open the dataset with the public/sharing link in a private browser tab.
  • The rendered color layer should render data like the histogram slider is at [100,150].

Issues:


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 find! Only see my one comment before merging :)

Comment on lines +52 to +55
Math.max(
currentLayerConfig.intensityRange[0],
currentLayerConfig.min != null ? currentLayerConfig.min : minimumInHistogramData,
),
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment which explains the intuition here? I think, I kind of got now, but I had think this through for a few minutes and I'm still not 100% sure. My take would be:
intensityRange is the range for which the histogram is plotted, right? By default, it's defined by the data type (e.g., uint16), but it can be configured per dataset layer.
That range serves as a boundary (i.e., the range cannot be increased by something else), but can still be narrowed down. If nothing else is provided, that down-narrowing is done by the sampled histogram. If the user specified a default min/max value, that is used instead.

Also interesting: intensityRange is more like a "theoretical" range which is not exceeded by the data (e.g., the microscope did only emit values between x and y), whereas min/max is more like a recommended cut-off to get a good rendering?

Did I get this right? 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

intensityRange is the range for which the histogram is plotted, right?

It is the exact opposite. The min/max is the range of the histogram (slider) while the intensityRange is the range to which the color values are clamped.

Is my new comment explaining the code good enough? I tried keeping it short.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense! Yes, should be good to go now :)

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.

Feel free to merge 🚢

Comment on lines +52 to +55
Math.max(
currentLayerConfig.intensityRange[0],
currentLayerConfig.min != null ? currentLayerConfig.min : minimumInHistogramData,
),
Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense! Yes, should be good to go now :)

@MichaelBuessemeyer MichaelBuessemeyer enabled auto-merge (squash) November 22, 2021 14:16
@MichaelBuessemeyer MichaelBuessemeyer merged commit 8b0c1d7 into master Nov 22, 2021
@MichaelBuessemeyer MichaelBuessemeyer deleted the fix-histogram-slider branch November 22, 2021 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial min/max values need to be changed once to get active
2 participants