Label loading fix#466
Conversation
✅ Deploy Preview for volview-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Cool. Gonna add a couple more tests... |
|
CI passed! Quick, merge before CI thinks better of it. But sersiouly, this PR makes somewhat pointless changes to to |
|
The pipeline results were intended to have an Either-like aspect to them, with at-runtime distinction via the |
floryst
left a comment
floryst
left a comment
There was a problem hiding this comment.
Splitting loading into a two-stage process (parse into loadable data sources, then load) has one minor downfall of having to wait for all resources to be ready prior to being loaded. This does make having streaming support all the more pertinent, so this two-stage approach is fine for now.
I wonder if we could have just gathered the config.json file instead of loading it and then apply it after the pipeline has run, similar to how we do it for the dicom data sources.
src/io/import/importDataSources.ts
Outdated
| const priorityOrImportResult = await Promise.all( | ||
| dataSources.map((r) => prioritizer.execute(r, importContext)) | ||
| ); |
There was a problem hiding this comment.
I think a simpler approach would be to add a loadPriority?: number field to ImportResult, then sort on the priority (defaulting to 0 if the key doesn't exist). The priority conversion pipeline + buckets can then be scrapped for a single sort call (or bucket approach). I think it'll be easier to understand and reason about that way.
There was a problem hiding this comment.
Yeah, what a mess. But better now.
Ran with your "do it like dicom sources" idea. handleConfig just calls done({ config ]). And the pipeline following code calls applyConfig on all resulting ImportResults with configs.
Removes favoring of existing labels in useAnnotationTool label deserialization. Favoring existing labels broke deserialization of edited labels. But, so config.JSON updates existing tools, add another pass to importDataSources so config.JSON are applied after session.zip has been applied. fixes #454
Test ensures config.json label props modify existing tool
Too flakey in CI.
|
Ready for review! Disabled the new e2e test, really flakey. |
|
Might not be due to your changes, but sometimes the rectangle annotation is filled-in, and other times it is not. 
|
|
Also, might not be your code, but the color of "artifact" (gray) doesn't match the color shown when it is drawn (yellow) 
|
|
The icon for rectangle includes control points...the icon for polyline does not 
|
|
@aylward Thanks for checking it out. I just removed the fillColor property from the default rectangle labels. Nobody liked the deformed polygon icon we had at first: Maybe we swap out rectangle for this: Menu of easly swapped icons here: |
+1 to using square-outline instead of the current one with control points. |
|
My concern is that there will be two similar squares next to one another -
one for the checkbox and one indicating the square shape.
Can we give the shapes a distinct background or better distinguish them
from the selection boxes?
I would also rather find a polyline with control boxes shown. I'll
search...
|
floryst
left a comment
floryst
left a comment
There was a problem hiding this comment.
Just 2 comments, but overall LGTM.
Let's not deal with the icons in this PR. We can continue the discussion in a separate issue.
|
LGTM! |
Label loading fix
3 participants
Removes favoring of existing labels in useAnnotationTool label deserialization.
Favoring existing labels broke deserialization of edited labels.
But, so config.JSON updates existing tools, add another pass to
importDataSources so config.JSON are applied after session.zip has been applied.
fixes #454