From 1cd55788e8221a1d4e1aad2e22b752eb63eff58f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20B=C3=BC=C3=9Femeyer?= Date: Fri, 6 Dec 2024 14:50:28 +0100 Subject: [PATCH] clean up code for pr review --- .../dataset/dataset_rotation_form_item.tsx | 31 ++++--- .../dataset/dataset_settings_view.tsx | 5 +- .../materials/plane_material_factory.ts | 7 +- .../dataset_layer_rotation_accessor.ts | 82 +++++++++---------- .../oxalis/model_initialization.ts | 6 +- frontend/javascripts/oxalis/store.ts | 6 +- .../left-border-tabs/layer_settings_tab.tsx | 26 +++--- 7 files changed, 85 insertions(+), 78 deletions(-) diff --git a/frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx b/frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx index 1df5e7631a..da9510e114 100644 --- a/frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx +++ b/frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx @@ -10,7 +10,7 @@ import { IDENTITY_TRANSFORM, getTranslationBackToOriginalPosition, AXIS_TO_TRANSFORM_INDEX, - haveAllLayersSameRotation, + doAllLayersHaveTheSameRotation, } from "oxalis/model/accessors/dataset_layer_rotation_accessor"; import BoundingBox from "oxalis/model/bucket_data_handling/bounding_box"; @@ -43,12 +43,16 @@ export const AxisRotationFormItem: React.FC = ({ () => getDatasetBoundingBoxFromLayers(dataLayers), [dataLayers], ); - // Update the transformations in case the user changes the dataset bounding box + // Update the transformations in case the user changes the dataset bounding box. useEffect(() => { - if (datasetBoundingBox == null || dataLayers[0].coordinateTransformations?.length !== 5) { + if ( + datasetBoundingBox == null || + dataLayers[0].coordinateTransformations?.length !== 5 || + !form + ) { return; } - const rotationValues = form?.getFieldValue(["datasetRotation"]); + const rotationValues = form.getFieldValue(["datasetRotation"]); const transformations = [ getTranslationToOrigin(datasetBoundingBox), getRotationMatrixAroundAxis("x", rotationValues["x"]), @@ -62,19 +66,22 @@ export const AxisRotationFormItem: React.FC = ({ coordinateTransformations: transformations, }; }); - form?.setFieldValue(["dataSource", "dataLayers"], dataLayersWithUpdatedTransforms); + form.setFieldValue(["dataSource", "dataLayers"], dataLayersWithUpdatedTransforms); }, [datasetBoundingBox, dataLayers, form]); const setMatrixRotationsForAllLayer = useCallback( (rotationInDegrees: number): void => { - const rotationInRadians = rotationInDegrees * (Math.PI / 180); - const rotationMatrix = getRotationMatrixAroundAxis(axis, rotationInRadians); - const dataLayers: APIDataLayer[] = form?.getFieldValue(["dataSource", "dataLayers"]); + if (!form) { + return; + } + const dataLayers: APIDataLayer[] = form.getFieldValue(["dataSource", "dataLayers"]); const datasetBoundingBox = getDatasetBoundingBoxFromLayers(dataLayers); if (datasetBoundingBox == null) { return; } + const rotationInRadians = rotationInDegrees * (Math.PI / 180); + const rotationMatrix = getRotationMatrixAroundAxis(axis, rotationInRadians); const dataLayersWithUpdatedTransforms: APIDataLayer[] = dataLayers.map((layer) => { let transformations = layer.coordinateTransformations; if (transformations == null || transformations.length !== 5) { @@ -92,7 +99,7 @@ export const AxisRotationFormItem: React.FC = ({ coordinateTransformations: transformations, }; }); - form?.setFieldValue(["dataSource", "dataLayers"], dataLayersWithUpdatedTransforms); + form.setFieldValue(["dataSource", "dataLayers"], dataLayersWithUpdatedTransforms); }, [axis, form], ); @@ -112,7 +119,7 @@ export const AxisRotationFormItem: React.FC = ({ { const dataLayers: APIDataLayer[] = form?.getFieldValue(["dataSource", "dataLayers"]); - const isRotationOnly = useMemo(() => haveAllLayersSameRotation(dataLayers), [dataLayers]); + const isRotationOnly = useMemo(() => doAllLayersHaveTheSameRotation(dataLayers), [dataLayers]); if (!isRotationOnly) { return ( @@ -158,7 +165,7 @@ export const AxisRotationSettingForDataset: React.FCTranslation back to the original position To easily enable this setting, delete all coordinateTransformations of all layers in the - advanced tab, save and reload. + advanced tab, save and reload the dataset settings. } > diff --git a/frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx b/frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx index 7daa5ddd25..5a39382052 100644 --- a/frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx +++ b/frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx @@ -45,7 +45,7 @@ import { defaultContext } from "@tanstack/react-query"; import { getReadableURLPart } from "oxalis/model/accessors/dataset_accessor"; import type { DatasetRotation } from "./dataset_rotation_form_item"; import { - haveAllLayersSameRotation, + doAllLayersHaveTheSameRotation, getRotationFromTransformation, } from "oxalis/model/accessors/dataset_layer_rotation_accessor"; @@ -200,7 +200,8 @@ class DatasetSettingsView extends React.PureComponent 1e-6 + Math.abs(cosOfAngle) > 1e-6 // Avoid division by zero ? Math.atan2(sinOfAngle, cosOfAngle) : sinOfAngle > 0 ? Math.PI / 2 @@ -92,7 +93,7 @@ export function getTranslationToOrigin(bbox: BoundingBox): AffineTransformation const center = bbox.getCenter(); const translationMatrix = new THREE.Matrix4() .makeTranslation(-center[0], -center[1], -center[2]) - .transpose(); + .transpose(); // Column-major to row-major return { type: "affine", matrix: flatToNestedMatrix(translationMatrix.toArray()) }; } @@ -100,7 +101,7 @@ export function getTranslationBackToOriginalPosition(bbox: BoundingBox): AffineT const center = bbox.getCenter(); const translationMatrix = new THREE.Matrix4() .makeTranslation(center[0], center[1], center[2]) - .transpose(); + .transpose(); // Column-major to row-major return { type: "affine", matrix: flatToNestedMatrix(translationMatrix.toArray()) }; } export function getRotationMatrixAroundAxis( @@ -109,13 +110,38 @@ export function getRotationMatrixAroundAxis( ): AffineTransformation { const euler = new THREE.Euler(); euler[axis] = angleInRadians; - const rotationMatrix = new THREE.Matrix4().makeRotationFromEuler(euler).transpose(); + const rotationMatrix = new THREE.Matrix4().makeRotationFromEuler(euler).transpose(); // Column-major to row-major const matrixWithoutNearlyZeroValues = rotationMatrix .toArray() + // Avoid nearly zero values due to floating point arithmetic inaccuracies. .map((value) => (Math.abs(value) < Number.EPSILON ? 0 : value)) as Matrix4x4; return { type: "affine", matrix: flatToNestedMatrix(matrixWithoutNearlyZeroValues) }; } +function memoizeWithThreeKeys(fn: (a: A, b: B, c: C) => T) { + const map = new MultiKeyMap(); + return (a: A, b: B, c: C): T => { + let res = map.get([a, b, c]); + if (res === undefined) { + res = fn(a, b, c); + map.set([a, b, c], res); + } + return res; + }; +} + +function memoizeWithTwoKeys(fn: (a: A, b: B) => T) { + const map = new MultiKeyMap(); + return (a: A, b: B): T => { + let res = map.get([a, b]); + if (res === undefined) { + res = fn(a, b); + map.set([a, b], res); + } + return res; + }; +} + // Returns the transforms (if they exist) for a layer as // they are defined in the dataset properties. function _getOriginalTransformsForLayerOrNull( @@ -150,7 +176,7 @@ export const getOriginalTransformsForLayerOrNull = memoizeWithTwoKeys( _getOriginalTransformsForLayerOrNull, ); -export function isLayerWithoutTransformConfigSupport(layer: APIDataLayer | APISkeletonLayer) { +export function isLayerWithoutTransformationConfigSupport(layer: APIDataLayer | APISkeletonLayer) { return ( layer.category === "skeleton" || (layer.category === "segmentation" && !layer.fallbackLayer) ); @@ -161,7 +187,7 @@ function _getTransformsForLayerOrNull( layer: APIDataLayer | APISkeletonLayer, nativelyRenderedLayerName: string | null, ): Transform | null { - if (isLayerWithoutTransformConfigSupport(layer)) { + if (isLayerWithoutTransformationConfigSupport(layer)) { return getTransformsForLayerWithoutTransformationConfigOrNull( dataset, nativelyRenderedLayerName, @@ -193,30 +219,6 @@ function _getTransformsForLayerOrNull( return chainTransforms(layerTransforms, inverseNativeTransforms); } -function memoizeWithThreeKeys(fn: (a: A, b: B, c: C) => T) { - const map = new MultiKeyMap(); - return (a: A, b: B, c: C): T => { - let res = map.get([a, b, c]); - if (res === undefined) { - res = fn(a, b, c); - map.set([a, b, c], res); - } - return res; - }; -} - -function memoizeWithTwoKeys(fn: (a: A, b: B) => T) { - const map = new MultiKeyMap(); - return (a: A, b: B): T => { - let res = map.get([a, b]); - if (res === undefined) { - res = fn(a, b); - map.set([a, b], res); - } - return res; - }; -} - export const getTransformsForLayerOrNull = memoizeWithThreeKeys(_getTransformsForLayerOrNull); export function getTransformsForLayer( dataset: APIDataset, @@ -237,28 +239,26 @@ function _getTransformsForLayerWithoutTransformationConfigOrNull( nativelyRenderedLayerName: string | null, ): Transform | null { const layers = dataset.dataSource.dataLayers; - const doAllLayersHaveTheSameRotation = haveAllLayersSameRotation(layers); + const allLayersSameRotation = doAllLayersHaveTheSameRotation(layers); if (nativelyRenderedLayerName == null) { // No layer is requested to be rendered natively. -> We can use each layer's transforms as is. - if (!doAllLayersHaveTheSameRotation) { + if (!allLayersSameRotation) { // If the dataset's layers do not have a consistent transformation (which only rotates the dataset), // we cannot guess what transformation should be applied to the layer. // As skeleton layer and volume layer without fallback don't have a transforms property currently. return null; } - // The skeleton layer needs transformed just like the other layers. Thus, we simply use the first usable layer. - // Filtering for a layer that might actually have transforms prevents an infinite loop - // between cyclic calls of _getTransformsForLayerWithoutTransformationConfigOrNull and getTransformsForLayerOrNull. + // The skeleton layer / volume layer without fallback needs transformed just like the other layers. + // Thus, we simply use the first usable layer which supports transforms. const usableReferenceLayer = layers.find( - (layer) => - layer.category === "color" || (layer.category === "segmentation" && layer.fallbackLayer), + (layer) => !isLayerWithoutTransformationConfigSupport(layer), ); const someLayersTransformsMaybe = usableReferenceLayer ? getTransformsForLayerOrNull(dataset, usableReferenceLayer, nativelyRenderedLayerName) : null; return someLayersTransformsMaybe; - } else if (nativelyRenderedLayerName != null && doAllLayersHaveTheSameRotation) { + } else if (nativelyRenderedLayerName != null && allLayersSameRotation) { // If all layers have the same transformations and at least one is rendered natively, this means that all layer should be rendered natively. return null; } @@ -303,7 +303,7 @@ function _getTransformsPerLayer( return transformsPerLayer; } -export const getTransformsPerLayer = memoizeOne(_getTransformsPerLayer); +export const getTransformsPerLayer = memoizeWithTwoKeys(_getTransformsPerLayer); export function getInverseSegmentationTransformer( state: OxalisState, @@ -367,7 +367,7 @@ function isRotationOnly(transformation?: AffineTransformation) { /* This function checks if all layers have the same transformation settings that represent * a translation to the dataset center and a rotation around each axis and a translation back. * All together this makes 5 affine transformation matrices. */ -function _haveAllLayersSameRotation(dataLayers: Array): boolean { +function _doAllLayersHaveTheSameRotation(dataLayers: Array): boolean { const firstDataLayerTransformations = dataLayers[0]?.coordinateTransformations; if (firstDataLayerTransformations == null || firstDataLayerTransformations.length === 0) { // No transformations in all layers compatible with setting a rotation for the whole dataset. @@ -411,4 +411,4 @@ function _haveAllLayersSameRotation(dataLayers: Array): boolean { return true; } -export const haveAllLayersSameRotation = _.memoize(_haveAllLayersSameRotation); +export const doAllLayersHaveTheSameRotation = _.memoize(_doAllLayersHaveTheSameRotation); diff --git a/frontend/javascripts/oxalis/model_initialization.ts b/frontend/javascripts/oxalis/model_initialization.ts index 5c530b9500..7b5c7190b6 100644 --- a/frontend/javascripts/oxalis/model_initialization.ts +++ b/frontend/javascripts/oxalis/model_initialization.ts @@ -105,7 +105,7 @@ import { isFeatureAllowedByPricingPlan, } from "admin/organization/pricing_plan_utils"; import { convertServerAdditionalAxesToFrontEnd } from "./model/reducers/reducer_helpers"; -import { haveAllLayersSameRotation } from "./model/accessors/dataset_layer_rotation_accessor"; +import { doAllLayersHaveTheSameRotation } from "./model/accessors/dataset_layer_rotation_accessor"; import type { Mutable } from "types/globals"; export const HANDLED_ERROR = "error_was_handled"; @@ -478,7 +478,7 @@ function getMergedDataLayersFromDatasetAndVolumeTracings( const originalLayers = dataset.dataSource.dataLayers; const newLayers = originalLayers.slice(); - const doAllLayersHaveTheSameRotation = haveAllLayersSameRotation(originalLayers); + const allLayersSameRotation = doAllLayersHaveTheSameRotation(originalLayers); for (const tracing of tracings) { // The tracing always contains the layer information for the user segmentation. @@ -497,7 +497,7 @@ function getMergedDataLayersFromDatasetAndVolumeTracings( const mags = tracing.mags || []; const tracingHasMagList = mags.length > 0; let coordinateTransformsMaybe = {}; - if (doAllLayersHaveTheSameRotation) { + if (allLayersSameRotation) { coordinateTransformsMaybe = { coordinateTransformations: originalLayers?.[0].coordinateTransformations, }; diff --git a/frontend/javascripts/oxalis/store.ts b/frontend/javascripts/oxalis/store.ts index f763a63c20..2fc44a5535 100644 --- a/frontend/javascripts/oxalis/store.ts +++ b/frontend/javascripts/oxalis/store.ts @@ -339,9 +339,9 @@ export type DatasetConfiguration = { // layer cancel each other out. // If nativelyRenderedLayerName is null, all layers are rendered // as their transforms property signal it. - // Currently, the skeleton layer does not have transforms as a stored - // property. So, to render the skeleton layer natively, nativelyRenderedLayerName - // can be set to null. + // Currently, the skeleton layer and volume layer without fallback do not have transforms + // as a stored property. So, to render the skeleton layer natively, + // nativelyRenderedLayerName can be set to null. readonly nativelyRenderedLayerName: string | null; }; diff --git a/frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx b/frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx index b6664e5649..71815ff8d7 100644 --- a/frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx +++ b/frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx @@ -127,10 +127,10 @@ import defaultState from "oxalis/default_state"; import { getTransformsForLayerOrNull, hasDatasetTransforms, - haveAllLayersSameRotation, + doAllLayersHaveTheSameRotation, getTransformsForLayer, isIdentityTransform, - isLayerWithoutTransformConfigSupport, + isLayerWithoutTransformationConfigSupport, } from "oxalis/model/accessors/dataset_layer_rotation_accessor"; import { invertTransform, @@ -226,7 +226,7 @@ function TransformationIcon({ layer }: { layer: APIDataLayer | APISkeletonLayer ); const showIcon = useSelector((state: OxalisState) => hasDatasetTransforms(state.dataset)); const doAllLayersHaveTheSameTransform = useSelector((state: OxalisState) => - haveAllLayersSameRotation(state.dataset.dataSource.dataLayers), + doAllLayersHaveTheSameRotation(state.dataset.dataSource.dataLayers), ); if (!showIcon) { return null; @@ -245,25 +245,25 @@ function TransformationIcon({ layer }: { layer: APIDataLayer | APISkeletonLayer }; const isDisabled = - (isRenderedNatively && !doAllLayersHaveTheSameTransform) || // Cannot toggle transforms on a layer into whose coordinate system other layer transform. - isLayerWithoutTransformConfigSupport(layer); - // Cannot toggle transformations on a skeleton layer as a skeleton layer cannot have transformations. - // Therefore, it cannot be used as a reference for other layers. - // The same goes for segmentation layers without fallback. - // Such layers can only transform according to transformations of other layers. + (isRenderedNatively && !doAllLayersHaveTheSameTransform) || + // Cannot toggle transformations on a skeleton layer as a skeleton layer cannot have transformations. + // Therefore, it cannot be used as a reference for other layers. + // The same goes for segmentation layers without fallback. + // Such layers can only transform according to transformations of other layers. + isLayerWithoutTransformationConfigSupport(layer); const toggleLayerTransforms = () => { const state = Store.getState(); // Get transform of layer. null is passed as nativelyRenderedLayerName to - // get the layers transform even in case it is currently rendered natively. - const currentTransform = getTransformsForLayer(state.dataset, layer, null); + // get the layers transform even in case the is currently rendered natively. + const layersTransforms = getTransformsForLayer(state.dataset, layer, null); // In case the layer is currently not rendered natively, the inverse of its transformation is going to be applied. // Therefore, we need to invert the transformation to get the correct new position. const transformWhichWillBeApplied = !isRenderedNatively - ? invertTransform(currentTransform) - : currentTransform; + ? invertTransform(layersTransforms) + : layersTransforms; const currentPosition = getPosition(state.flycam); const newPosition = transformPointUnscaled(transformWhichWillBeApplied)(currentPosition);