Support other units than nm for datasets #7783
Conversation
- make keep the base unit internally to nm - use configured unit for 3d space / voxel space - WIP: post fix all relevant function with Nm or Vx depending on their unit to make this as explicit as possible
- current state: broken; nothing is rendered
- add a lot of debugging output for testing [ci skip]
…in nm) - [ci skip] - Dont ignore far plane calculation
| const datasetExtent = getDatasetExtentInDatasourceUnit(Store.getState().dataset); | ||
| console.log( | ||
| "CameraController.componentDidMount: getDatasetExtentInDatasourceUnit", | ||
| datasetExtent, | ||
| ); | ||
| const diagonalDatasetExtent = Math.sqrt( | ||
| datasetExtent.width ** 2 + datasetExtent.height ** 2 + datasetExtent.depth ** 2, | ||
| ); | ||
| this.props.cameras[OrthoViews.TDView].far = diagonalDatasetExtent * 2; | ||
| const far = Math.max(8000000, diagonalDatasetExtent * 2); | ||
|
|
||
| for (const cam of _.values(this.props.cameras)) { | ||
| cam.near = 0; | ||
| cam.far = far; | ||
| } | ||
| // This value is correct | ||
| this.props.cameras[OrthoViews.TDView].far = far; |
There was a problem hiding this comment.
Previously, the value calculated for the far plane was overwritten in lines 103-108 after the 3d viewport was properly initialized. I modified the code to the larger of both values is kept.
- Full support for uncommon units is missing for 2d and 3d
…g to nm [ci skip]
…nly in 1d && fix lots of tests [ci skip]
- WIP: Fix tests now that cm is supported
And remove datasetScale/voxelSize uniform from shaders as it was unused
|
I also renamed |
There was a problem hiding this comment.
I just had another look at the backend code and could not locate anything bug/mistake / improvement 🎉
Besides that, here are my pending comments regarding the feedback from @philippotto
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/download_modal_view.tsx
Outdated
Show resolved
Hide resolved
| @@ -1027,6 +1027,13 @@ function _getLoadChunksTasks( | |||
| // Compute vertex normals to achieve smooth shading | |||
| bufferGeometries.forEach((geometry) => geometry.computeVertexNormals()); | |||
|
|
|||
| // Check if the mesh scale is different to the dataset scale and warn in the console to make debugging easier in such a case. | |||
| if (!_.isEqual(scale, dataset.dataSource.scale.factor)) { | |||
There was a problem hiding this comment.
Precomputed meshes just work fine.
But adhoc meshing seems broken. See my comment / "bug report" below -> good catch
frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts
Outdated
Show resolved
Hide resolved
|
Sorry to ping you again @philippotto. Could you please check the newest changes regarding using the long unit name internally instead of the short one in the frontend? |
|
Newest backend change LGTM 👍 |
The changes look good 👍 However, I would suggest to rename |
|
@fm3 Mergy merge today? |
* Revert "Revert "Support other units than nm for datasets (#7783)" (#7896)" This reverts commit f874f68. * always pass short unit version to findBestUnitForFormatting * in zarr streaming served datasource-properties.json, use old voxel size format * make typing for UnitDimension / findBestUnitForFormatting more strict * make type of unit param for adjustUnitToDimension more restrictive --------- Co-authored-by: Michael Büßemeyer <[email protected]>
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
cmas a format to be formatted to.After this PR / Follow up
Open Questions:
...InDatasourceUnit...InDatasetUnit...InUnit1.23456789cm²is formatted to0.001m²in case a decimal precision of 4 is wanted. Is this how it should be or is this unwanted?preferShorterDecimalsof theformatNumberToUnitfunction. Here is an examplepreferShorterDecimalsfalse as default as it does not cut off that much precision! -> Agreed upon.preferShorterDecimals = falseleads to0.1nm³getting formatted to100000000.0 pm³. This is very likely unwanted?!. I could refactor the formatting to always prefer the unit that has fewer digits. But this might require some additional string operations thus making the code potentially slower -> In such a case: measure and look at how frequently the code is called.Features to test:
mmseems correct, but the vx is off.api...getViewportDataIssues:
(Please delete unneeded items, merge only when none are left open)