Multi-scene collections #362
Conversation
…nimated/vole-app into feature/multi-scene
|
frasercl
requested review from
toloudis and
ShrimpCryptid
and removed request for
a team
| const [image, setImage] = useState<Volume | null>(null); | ||
| const imageUrlRef = useRef<string | string[]>(""); | ||
| const imageUrlRef = useRef<string | (string | string[])[]>(""); |
There was a problem hiding this comment.
Nit: this T | (T | T[])[] i see used a couple of places. Wondering about some readable alias like "valueOrArrayOrArray2D". even T | T[] | T[][] looks simpler to read
There was a problem hiding this comment.
I even wonder if there's a way to formalize it with a simple construct like
type Scene {
sources: string[];
}
instead of carrying these weird arrays around.
Then the url parsing can just fill an array of Scenes.
There was a problem hiding this comment.
did roughly the second thing above
website/utils/url_utils.ts
Outdated
| const imageUrls = tryDecodeURLList(params.url) ?? decodeURL(params.url); | ||
| const firstUrl = Array.isArray(imageUrls) ? imageUrls[0] : imageUrls; | ||
| const sceneUrls = tryDecodeURLList(params.url, " ") ?? [params.url]; | ||
| const sourceUrls = sceneUrls.map((scene) => tryDecodeURLList(scene) ?? [decodeURL(scene)]); |
There was a problem hiding this comment.
is this supposed to give you every source url of every scene? I guess I don't know what tryDecodeUrlList can return . I assume decodeURL returns a single string?
website/utils/url_utils.ts
Outdated
|
|
||
| const firstScene = sourceUrls[0]; | ||
| const firstUrl = Array.isArray(firstScene) ? firstScene[0] : firstScene; | ||
| const imageUrls = sourceUrls.length === 1 ? (sourceUrls[0].length === 1 ? firstScene[0] : firstScene) : sourceUrls; |
There was a problem hiding this comment.
it has become too hard to parse these 5 lines. I think types on the variable declarations would help to understand. And maybe rename imageUrls and firstUrl to be imageUrlsOfFirstScene and firstUrlOfFirstScene? if that's even accurate?
There was a problem hiding this comment.
My little refactor didn't make this much better, but I did add some docs and a type signature. Let me know if there's more I can do?
| props; | ||
| const clippingPanelTall = numTimesteps > 1 && numScenes > 1 && viewMode === ViewMode.threeD; |
There was a problem hiding this comment.
This check happens in two places... For DRY, should it just be a single flag that's passed in as a prop?
There was a problem hiding this comment.
My thought here was that, since this property is derivable from other passed properties, it was better not to pass it in explicitly and risk an "invalid" value. Though this value has less of a strong notion of "invalid" than, say, a string along with an isAllCaps: boolean value. So maybe it would be better?
| const urls = (appProps.imageUrl as MultisceneUrls).scenes ?? [appProps.imageUrl]; | ||
| const sceneUrlsUnencoded = urls.map((scene) => (Array.isArray(scene) ? scene.join(",") : scene)); | ||
| navigation(`/viewer?url=${encodeURIComponent(sceneUrlsUnencoded.join(" "))}`, { |
There was a problem hiding this comment.
If this code is copied with AppWrapper maybe it's worth putting in a function?
Review time: medium (~20-30min)
Problem
Scientists want a way to view multiple images collected along a "scenes" dimension. This PR gets us to basic support for that.
Resolves #351: adds a new "Scene" slider to the clipping panel.
Additionally, resolves allen-cell-animated/vole-core#275. Most of the work on this ticket was finished in allen-cell-animated/vole-core#279 — that PR described the remaining work as creating a new data structure or framework to gather multiple data sources into a single multi-scene collection. This PR includes that: the
SceneStoreclass, described below.Solution
SceneStoreclass, which manages creating and swapping out loaders for different scenes, and which replaces the single loader that theAppcomponent used to hold onto.Appprops to support describing multi-scene collections, and new semantics to URL parameters to parse out those propsRemaining work
This implementation deliberately performs minimal validation. For example, it will fall over when successive scenes have a different number of channels. We consider this acceptable while we're demoing this feature to scientists, since this feature isn't exposed to users in any prominent way. This remaining work is captured in allen-cell-animated/vole-core#277.
Steps to verify
npm install,npm start?url=parameter to the URL, and join multiple URLs with+. For example: http://localhost:9020/viewer?url=https%3A%2F%2Fanimatedcell-test-data.s3.us-west-2.amazonaws.com%2Fvariance%2F1.zarr+https%3A%2F%2Fanimatedcell-test-data.s3.us-west-2.amazonaws.com%2Fvariance%2F2.zarr+https%3A%2F%2Fanimatedcell-test-data.s3.us-west-2.amazonaws.com%2Fvariance%2F3.zarrScreenshot