Add possibility to edit min and max of intensity range #4630
Conversation
MichaelBuessemeyer
changed the title
There was a problem hiding this comment.
Cool PR, looks really good and the functionality is there, but I encountered some issues during testing that I wanted to note down:
- I'm not sure about the scientific notation feature. Try entering a large max value like 400123 by hand - you end up with 4.0012e+43 which is not what you wanted to type. It's confusing that the value changes while you type. If we still want the scientific notation feature to avoid overly long numbers, I would suggest to add a mechanism to confirm the entered number by clicking on a button. Then, the number could change to scientific notation. However, I don't think this is very important.
- Somehow, I ended up with NaN in the min field, but I can't reproduce now :/ Disregard if you also can't reproduce.
- I tried to delete the whole min value, which crashed the site. Aftwerwards, I couldn't reopen the histogram without webKnossos crashing. I checked the store state and a min value of
""was persisted which I had to fix through the dev console. - Not very high priority, but there is a light blue area (triangular shape) to the right of the largest histogram value when the configured max is even larger. See the picture:
|
Thanks a lot for finding those bugs 🐛. Hopefully, I fixed them all. Could you please check that again?
Thus we should not support displying in scientific notation? Is that what you meant? |
Yes, I don't think it is very important, and I don't see an easy way to implement it with good UX, so I wouldn't add it for now. |
| const activeRegionRightLimit = Math.min(histogramMax, intensityRangeMax); | ||
| const activeRegionLeftLimit = Math.max(histogramMin, intensityRangeMin); | ||
| activeRegion.lineTo(((activeRegionRightLimit - minRange) / fullLength) * canvasWidth, 0); | ||
| activeRegion.lineTo(((activeRegionLeftLimit - minRange) / fullLength) * canvasWidth, 0); |
There was a problem hiding this comment.
@MichaelBuessemeyer My assumption would be that histogramMax is always <= intensityRangeMax and therefore, it should be sufficient to simply use histogramMax in the activeRegion.lineTo call. Is that assumption correct?
There was a problem hiding this comment.
Maybe I was confusing the histogramMax and intensityRangeMax. Hmm, could you distinguish what each of those is again for me? Maybe we can find more descriptive names that make the difference more clear.
There was a problem hiding this comment.
I think you really confused both of them:
histogramMaxis the maximum color value that is included in the histogram data send by the backend.intensityRangeMaxis the maximum color value of the displayed color range and is equal to the higher value of the slide. What's new now is that this value can be greater thanhistogramMaxas you can see it in your example:
To be honest I think the names are pretty good as the intensityRange is the range given by the slider and thus intensityRangeMax as the upper limit of that range is not far-fetched (in my opinion) -> Feel free to argue about that.
The same goes for histogramMax as this is the highest color value included in the histogram data.
There was a problem hiding this comment.
Ah understood, that makes sense, sorry for the confusion :)
There was a problem hiding this comment.
Thanks for your fixes @MichaelBuessemeyer, I tested again and noticed some more small things, but we're getting there 👍
- The behavior when entering an entirely new number is kind of weird. For example, the max value is set to 100, now you enter 255 instead. When entering the 2, the max slider of the histogram is automatically set to 2, and when entering the 55 afterwards, the max slider remains at 2. However, the max slider was set to 100 before and it's a little unexpected that this changed (although I understand why it happened). A possible solution for this could be to debounce the onChange of the input field, so that the histogram only changes after a second of no change or so. What do you think?
- The edit histogram button should not be available for segmentation layers (or layers without a histogram if there are any others?)
- [Low Pri] Maybe it's an easy fix, if not nevermind: The two vertical side lines of the active area are not vertical, but slightly angled to the middle. To see this enter 100 and 102 for min and max and look at the active area.
I tried to tackle this by only updating when the events
I cannot find the cause for this 😕 @daniel-wer Could you please check the small updates I just pushed? |
|
@daniel-wer Thanks for the suggestions. I just applied them. |
This PR adds the possibility for the user to edit the minimum and maximum possible value for the intensity range.
The use case is described in #4526.
URL of deployed dev instance (used for testing):
Steps to test:
Open a dataset (view mode is enough)
Toggle the edit function for min and max. The button is on the left of the reload-layer button.
enter some desired values
the histogram and the
intensityRangeslider's range should adjust to the entered values.then adjust the
intensityRangeand you should be able to see something again :)please try breaking the feature by entering some random and uncommon values
Issues:
[ ] Updated (unreleased) migration guide if applicable[ ] Updated documentation if applicable[ ] Adapted wk-connect if datastore API changes[ ] Needs datastore update after deployment