Scale Bar #158
Scale Bar #158
Conversation
There was a problem hiding this comment.
Looks good! Made a view comments, principally I'd like to avoid copying fields from omexml directly and probably add something like loader.physicalSizes where we store the unit and value for each of the available physical dimensions (XYZ). This way, whatever metadata ends up like for zarr, we don't need to create some odd field on the loader that is an artifact of other metadata source (tiff omexml).
src/layers/ScaleBarLayer.js
Outdated
| switch (position) { | ||
| case 'bottom-right': { | ||
| const yCoord = | ||
| boundingBox[2][1] - (boundingBox[2][1] - boundingBox[0][1]) * 0.085; |
There was a problem hiding this comment.
what is the 0.085 doing? maybe add this as an arg in getPosition with this default.
| PhysicalSizeXUnit && PhysicalSizeXUnit.includes('µm') | ||
| ? 'µm' | ||
| : this.PhysicalSizeXUnit; |
There was a problem hiding this comment.
Can you describe what the 'odd behavior' is?
There was a problem hiding this comment.
I'll leave a clearer note.
There was a problem hiding this comment.
There was a prepended character. Could have been an error but the µm is kind of wonky so it could be a python-javascript difference.
src/views/DetailView.js
Outdated
| @@ -18,6 +19,17 @@ export default class DetailView extends VivView { | |||
| id: `${loader.type}${getVivId(id)}`, | |||
| viewportId: id | |||
| }); | |||
| return [layer]; | |||
| const { PhysicalSizeXUnit, PhysicalSizeX } = loader; | |||
There was a problem hiding this comment.
I'd prefer that if these are on the loaders we use camelcase like other properties, rather than copying the omexml keys directly. Similar to tileSize, we assume that the PhysicalSizeX and PhysicalSizeY are the same for the image, so maybe we could just have the property physicalSize which is an object:
const { unit, value } = loader.physicalSize;or if we want to add all sizes and scale bar just assumes x:
const { x, y, z } = loader.physicalSizes;
const { unit, value } = x;There was a problem hiding this comment.
this will make it easy to implement on the zarr loader.
There was a problem hiding this comment.
You could even check size units in x and y and console.warn if they are different.
There was a problem hiding this comment.
Thanks for this. This was the thorniest point so I'm glad to have the feedback.
src/views/DetailView.js
Outdated
| const { loader } = props; | ||
| const { id } = this; | ||
| const thisViewState = viewStates[id]; |
There was a problem hiding this comment.
perhaps layerViewState or viewStateUpdate or viewState? Confusing to prefix with this in javascript because it is a keyword...
src/views/SideBySideView.js
Outdated
| const { PhysicalSizeXUnit, PhysicalSizeX } = loader; | ||
| const scaleBarLayer = | ||
| PhysicalSizeXUnit && PhysicalSizeX | ||
| ? new ScaleBarLayer({ | ||
| id: getVivId(id), | ||
| loader, | ||
| PhysicalSizeXUnit, | ||
| PhysicalSizeX, | ||
| viewState: thisViewState | ||
| }) | ||
| : null; | ||
| return [detailLayer, border, scaleBarLayer]; |
There was a problem hiding this comment.
It would be clearer to me what's going on if we just pushed layers to an empty array, rather than adding a null here.
const layers = [];
// ...
layers.push(detailLayer);
// ...
layers.push(border);
// ...
if (PhysicalSizeXUnit && PhysicalSizeX) {
// ...
layers.push(scaleBarLayer);
}| const layers = view.getLayers({ props: { loader } }); | ||
| const layers = view.getLayers({ | ||
| props: { | ||
| loader: { ...loader, PhysicalSizeXUnit: 'cm', PhysicalSizeX: 0.5 } |
There was a problem hiding this comment.
why not just define these on the loader above?
There was a problem hiding this comment.
The base test just checks for id-passing. This one checks that proper layers are rendered, for which this is needed and in the former case it is not. The OverviewView, for example, doesn't technically need this information.
|
|
||
| generateViewTests(SideBySideView, defaultArguments); | ||
|
|
||
| test(`SideBySideView layer type and props check`, t => { | ||
| const view = new SideBySideView(defaultArguments); | ||
| const loader = { type: 'loads', isPyramid: true }; | ||
| const layers = view.getLayers({ | ||
| props: { loader }, | ||
| props: { | ||
| loader: { ...loader, PhysicalSizeXUnit: 'cm', PhysicalSizeX: 0.5 } |
There was a problem hiding this comment.
same here. probably just create the loader object initially in one place.
This PR adds a scale bar:
I would have liked the API be smaller (i.e no
viewState) for the layer but it is not possible unfortunately: visgl/deck.gl#4504Fixes #60.