-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome 👍 code looks good and it works great, too.
- Build auto conversion for huge scales into a better fitting unit.
I think, it's not really necessary for this PR. we can advise users to do this manually if problems arise.
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 <michael.buessemeyer@student.hpi.de>
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
cm
as a format to be formatted to.After this PR / Follow up
Open Questions:
...InDatasourceUnit
...InDatasetUnit
...InUnit
1.23456789cm²
is formatted to0.001m²
in case a decimal precision of 4 is wanted. Is this how it should be or is this unwanted?preferShorterDecimals
of theformatNumberToUnit
function. Here is an examplepreferShorterDecimals
false as default as it does not cut off that much precision! -> Agreed upon.preferShorterDecimals = false
leads 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:
mm
seems correct, but the vx is off.api...getViewportData
Issues:
(Please delete unneeded items, merge only when none are left open)