Improve color palette for tree colors #8228
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several updates to the WEBKNOSSOS application, including a new feature for pasting remote URIs from Neuroglancer, asynchronous reading of image files, improved error messages for job management, and the removal of bounding box restrictions for super users. Additionally, it enhances default colors for skeleton trees and fixes multiple bugs related to tree deletion and NML file imports. The integration of Google Analytics has also been removed. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
CHANGELOG.unreleased.md (1)
20-20: Enhance the changelog entry to better reflect the changes and their impactThe current entry "Improved the default colors for skeleton trees" could be more descriptive about the specific improvements and their user impact, especially given that this addresses user feedback about color contrast issues.
Consider expanding the entry to something like:
- Improved the default colors for skeleton trees. [#8228](https://github.com/scalableminds/webknossos/pull/8228) + Enhanced the color palette for skeleton trees to improve contrast between dark and bright colors, making trees more visually distinguishable. [#8228](https://github.com/scalableminds/webknossos/pull/8228)frontend/javascripts/libs/color_generator.ts (1)
Line range hint
71-88: Consider optimizing random color generationThe current implementation shuffles colors for each request. Consider caching the shuffled colors to improve performance when repeatedly requesting random colors.
const ColorGenerator = { + _shuffledCache: null as Vector3[] | null, + distinctColorForId(id: number): Vector3 { return rgbs[(id - 1 + rgbs.length) % rgbs.length]; }, getNRandomColors(n: number): Vector3[] { - let shuffledColors: Vector3[] = []; + // Reset cache if it doesn't exist + if (!this._shuffledCache) { + this._shuffledCache = _.shuffle([...rgbs]); + } + + let result: Vector3[] = []; let remainingColorCount = n; while (remainingColorCount > 0) { // Take the first k colors const batchSize = Math.min(remainingColorCount, rgbs.length); - shuffledColors = shuffledColors.concat(rgbs.slice(0, batchSize)); + result = result.concat(this._shuffledCache.slice(0, batchSize)); remainingColorCount -= batchSize; + + // Reshuffle cache if we need more colors + if (remainingColorCount > 0) { + this._shuffledCache = _.shuffle([...rgbs]); + } } - return _.shuffle(shuffledColors); + return result; }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/libs/color_generator.ts(2 hunks)
🔇 Additional comments (1)
frontend/javascripts/libs/color_generator.ts (1)
68-68: LGTM! Type-safe color mapping implementation
The color mapping implementation correctly:
- Groups RGB values into triplets
- Normalizes values to 0-1 range
- Ensures type safety with Vector3 assertion
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts (1)
841-841: Avoid hardcoding default color values in testsIn line 841, the test asserts that the new tree's color is
[0, 0, 1]. Hardcoding the default color value may make the test brittle if the default color changes in the future.Consider retrieving the default color from the relevant configuration or constants to ensure the test remains valid even if the default color changes. For example:
treeId: 2, - color: [0, 0, 1], + color: DEFAULT_TREE_COLOR, });Where
DEFAULT_TREE_COLORis imported from the module where the default color is defined.frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (2)
Line range hint
377-385: Simplify the Toast message handlingWrapping the Toast message in
<React.Fragment>is unnecessary since it contains only a single text node.Consider simplifying the code by removing the
<React.Fragment>wrapper:if (!segment.somePosition) { - Toast.info( - <React.Fragment> - Cannot load a mesh for this segment, because its position is unknown. - </React.Fragment>, - ); + Toast.info('Cannot load a mesh for this segment, because its position is unknown.'); hideContextMenu(); return; }This makes the code cleaner and easier to read.
Line range hint
429-437: Simplify the Toast message handlingSimilar to a previous comment, the use of
<React.Fragment>here is unnecessary.Simplify the code by removing the fragment:
if (!segment.somePosition) { - Toast.info( - <React.Fragment> - Cannot load a mesh for this segment, because its position is unknown. - </React.Fragment>, - ); + Toast.info('Cannot load a mesh for this segment, because its position is unknown.'); hideContextMenu(); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/libs/color_generator.ts(2 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(1 hunks)frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/libs/color_generator.ts
- CHANGELOG.unreleased.md
🔇 Additional comments (8)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (8)
65-65: Consistent spacing for icon alignment
The addition of marginLeft: 1 improves the alignment of the colored dot icon. Ensure that this margin aligns well with other elements in different contexts and screen sizes.
Line range hint 333-334: Correctly display context menu based on selection
The logic correctly displays the multi-select menu when multiple segments are selected and the current segment is among them.
Line range hint 388-392: Ensure proper handling of optional parameters in withMappingActivationConfirmation
You're passing mappingName, which may be undefined if currentMeshFile is null. Ensure that withMappingActivationConfirmation can handle an undefined mappingName without causing errors.
Line range hint 407-407: Improved user feedback with descriptive tooltip
Providing a clear tooltip message when there is no mesh file enhances user feedback.
Line range hint 447-450: Enhanced user experience with dynamic menu item
The addition of isActiveSegment and dynamic title provides immediate feedback to the user about the segment's activation state.
Line range hint 471-478: Display metadata icon for segments with assigned properties
Including an icon to indicate when a segment has metadata properties improves user awareness and interaction.
Line range hint 543-544: Efficient determination of centered segment
The addition efficiently checks whether the segment is currently centered in the view.
Line range hint 834-840: Correctly disable 'Delete Segment's Data' option when unsupported
The updated disabled condition ensures that the 'Delete Segment's Data' menu item is only enabled when supported, preventing unintended actions.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)