-
Notifications
You must be signed in to change notification settings - Fork 24
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
Colors for mappings #2965
Colors for mappings #2965
Conversation
…olor of the first equivalence class now, add option to hide unmapped ids (#2173)
…segment transparent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Works like a charm 👍
- I agree that a default color should be used when there are not enough colors, but the hide-option is not set. If that's done, the PR should be ready to merge :)
@@ -186,6 +191,10 @@ class PlaneMaterialFactory extends AbstractPlaneMaterialFactory { | |||
type: "t", | |||
value: mappingLookupTexture, | |||
}; | |||
this.uniforms[sanitizeName(`${segmentationLayer.name}_mapping_color_texture`)] = { | |||
type: "t", | |||
value: mappingColorTexture, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope the increased texture uniform count will not become a problem for older hardware :/ Can you test on browserstack whether iPad still works? 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mappings will be disallowed (and the textures won't be attached, see the check a couple of lines above which is not visible here) if there are not enough textures available (Model.isMappingSupported
), so this shouldn't be a problem, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's right. However, the uniform is always defined in JS-land. I'm not sure whether this has implications on some systems. But probably not. We'll see :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uniform "definition" is inside a if (segmentationLayer != null && Model.isMappingSupported) {
check, so it should not have any implications :)
getLargestSegmentId(): number { | ||
const segmentationLayer = getLayerByName(Store.getState().dataset, this.layerName); | ||
if (segmentationLayer.category !== "segmentation") { | ||
throw new Error("Mappings class must be instantiated with a segmenation layer."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
segmentation
updateMappingColorTexture(mappingColors: ?Array<number>) { | ||
mappingColors = mappingColors || []; | ||
const float32Colors = new Float32Array( | ||
MAPPING_COLOR_TEXTURE_WIDTH * MAPPING_COLOR_TEXTURE_WIDTH, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MAPPING_COLOR_TEXTURE_WIDTH ** 2
is shorter :)
@@ -119,7 +124,7 @@ ${compileShader( | |||
getRelativeCoords, | |||
getWorldCoordUVW, | |||
getMaybeFilteredColorOrFallback, | |||
convertCellIdToRGB, | |||
hasSegmentation ? convertCellIdToRGB : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@philippotto I applied your PR feedback, most notably a default color is now used, when there are not enough colors specified. I'd go on and merge this today :) |
Nice, sounds excellent 👍 |
This PR adds the possibility to add custom colors for mappings as well as to hide unmapped segments.
Up to 256 colors (hsv hue values) can be specified which are then used in order to color the equivalence classes of the mapping. The interface using the API is not as clean yet (see my description in https://discuss.webknossos.org/t/colors-for-mappings/602/27?u=dwerner) but as the single user of that feature will be using the json mapping interface, I think that is fine for now. We'll hopefully align the json and API mappings interface in the near future.
Note that the are now two ways that segments can end up to be transparent. If the user specifies the
hideUnmappedIds
flag and doesn't map a segment id, it will be transparent. Also if the user specifies not enough entries in themappingColors
array (e.g.: 3 equivalence classes, but only 2 hue values), the segments of the equivalence classes without a color will be transparent.Thinking about it while writing this PR description, maybe it would be better to have a default color in that second case. If I remember correctly I introduced this behavior in the beginning to solve the
hideUnmappedIds
feature, which I solved differently now ^^URL of deployed dev instance (used for testing):
Steps to test:
Open a tracing using the ROI2017_wkw dataset.
Locally, add a mapping file like this in the mappings folder of the segmentation folder of a dataset (don't forget to add the mapping name in the datasource.json):
mappings.activate("colors")
Mappings without custom colors should work as before
Disabling/Enabling mappings should work as before
Issues:
Updated migration guide if applicable