feature: Annotation data model (annotations pt. 1) #499
+487
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ShrimpCryptid
added
new feature
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
ShrimpCryptid
commented
Dec 17, 2024
Comment on lines
+257
to
+265
| * Wrapper around the SharedWorkerPool method `getMotionDeltas`. Returns a | ||
| * debounced motion delta array for the given dataset and vector field | ||
| * configuration. | ||
| * @param dataset The dataset to calculate motion deltas for. | ||
| * @param workerPool The worker pool to use for asynchronous calculations. | ||
| * @param config The vector field configuration to use. | ||
| * @param debounceMs The debounce time in milliseconds. Defaults to 100ms. | ||
| * @returns The motion delta array or `null` if the dataset is invalid. Data will be | ||
| * asynchronously updated as calculations complete. | ||
| * @returns The motion delta array or `null` if the dataset is invalid. Data | ||
| * will be asynchronously updated as calculations complete. |
There was a problem hiding this comment.
Tidied this comment slightly
fix: Linting refactor: Cleanup comments in AnnotationData
refactor: Removed unused method from AnnotationData
fix: Linting refactor: Renamed methods for consistency doc: Updated comment refactor: Code cleanup
ShrimpCryptid
force-pushed
the
feature/annotation-data
branch
from
6ed64e3 to
4fc258f
Compare
ShrimpCryptid
commented
Dec 20, 2024
interim17
approved these changes
Dec 20, 2024
| } ; | ||
|
|
||
| export const useAnnotations = (): AnnotationState => { | ||
| const annotationData = useConstructor(() => new AnnotationData()); |
There was a problem hiding this comment.
Can't remember if I looked at this before but it's a nifty util
frasercl
approved these changes
Jan 3, 2025
There was a problem hiding this comment.
Liking this overall! Approving preemptively since I left it hanging over the break and because all my comments are optional/matters of opinion, and you're the expert on where this code goes from here and what it needs. A couple of those optional comments are relatively substantial though.
| import Dataset from "../Dataset"; | ||
| import SharedWorkerPool from "../workers/SharedWorkerPool"; | ||
|
|
||
| // TODO: Move this to a folder outside of `colorizer`. |
There was a problem hiding this comment.
+1 to this - still like the idea of keeping colorizer independent of React
Comment on lines
+113
to
+115
| getLabels(): LabelData[] { | ||
| return [...this.labelData]; | ||
| } |
There was a problem hiding this comment.
- Why does this copy, rather than just returning
this.labelData? - Is it a problem that this shallow-copies? Modifying a member of the returned array will (if I'm not mistaken) modify the same
LabelDataobject stored inthis.labelData.
There was a problem hiding this comment.
I guess I wanted to protect the label data and make it readonly, but you're 100% right that you can still mutate the underlying objects. At the very least this prevents changing of the label ordering which would mess up the UI rendering.
Co-authored-by: Cameron Fraser <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Part 1 of 5ish for #484, annotation prototyping!
This PR implements the data model used for annotations. It syncs a pure TS class with React state using a hook to encapsulate all changes. This isn't wired up to anything yet in this PR, but will be used extensively in the next couple PRs for this feature.
Note that this change is missing track information, which means the API will change slightly in the near future.
Estimated review size: medium, 30 minutes (line count is higher due to unit testing)
(Also, you can see the end result of all 5 of the annotation prototype PRs here: https://allen-cell-animated.github.io/timelapse-colorizer/pr-preview-internal/pr-498/)
Solution
AnnotationDataclass, which stores the current annotation state in the app.useAnnotationshook inreact_utils.ts.AnnotationData.AnnotationDataso state is updated accordingly.Type of change