Skip to content
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

Make isosurfaces more robust #5102

Merged
merged 7 commits into from
Jan 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
[Commits](https://github.com/scalableminds/webknossos/compare/21.02.0...HEAD)

### Added
- The "Meshes" tab was overhauled, so that it displays generated isosurfaces and imported meshes. Generated isosurfaces can be jumped to, reloaded, downloaded and removed. [#4917](https://github.com/scalableminds/webknossos/pull/4917)
- Added an explicit `/signup` (or `/auth/signup`) route. [#5091](https://github.com/scalableminds/webknossos/pull/5091/files)

### Changed
- Make the isosurface feature in the meshes tab more robust. If a request fails, a retry is initiated. [#5102](https://github.com/scalableminds/webknossos/pull/5102)
- Support for the old invite links was removed. These contained the organization name in the URL. The new links contain a token (can be generated in the users view). For instances with a single organization the old invite links should still work. [#5091](https://github.com/scalableminds/webknossos/pull/5091/files)
- Users are no longer allowed to deactivate their own accounts. [#5070](https://github.com/scalableminds/webknossos/pull/5070)

### Fixed
-

### Removed
-
- The isosurface setting was removed. Instead, isosurfaces can be generated via the "Meshes" tab. Also note that the Shift+Click binding for generating an isosurface was removed (for now). Please refer to the "Meshes" tab, too. [#4917](https://github.com/scalableminds/webknossos/pull/4917)
10 changes: 7 additions & 3 deletions frontend/javascripts/oxalis/model/reducers/annotation_reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,22 +116,26 @@ function AnnotationReducer(state: OxalisState, action: Action): OxalisState {

case "ADD_ISOSURFACE": {
const { cellId, seedPosition } = action;
return updateKey2(state, "isosurfaces", cellId.toString(), {
// $FlowIgnore[incompatible-call] updateKey has problems with updating Objects as Dictionaries
return updateKey2(state, "isosurfaces", cellId, {
segmentId: cellId,
seedPosition,
isLoading: false,
});
}

case "START_REFRESHING_ISOSURFACE": {
const { cellId } = action;
return updateKey2(state, "isosurfaces", cellId.toString(), {
// $FlowIgnore[incompatible-call] updateKey has problems with updating Objects as Dictionaries
return updateKey2(state, "isosurfaces", cellId, {
isLoading: true,
});
}

case "FINISHED_REFRESHING_ISOSURFACE": {
const { cellId } = action;
return updateKey2(state, "isosurfaces", cellId.toString(), {
// $FlowIgnore[incompatible-call] updateKey has problems with updating Objects as Dictionaries
return updateKey2(state, "isosurfaces", cellId, {
isLoading: false,
});
}
Expand Down
85 changes: 56 additions & 29 deletions frontend/javascripts/oxalis/model/sagas/isosurface_saga.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// @flow
import { saveAs } from "file-saver";

import { sleep } from "libs/utils";
import ErrorHandling from "libs/error_handling";
import type { APIDataset } from "types/api_flow_types";
import { ResolutionInfo, getResolutionInfo } from "oxalis/model/accessors/dataset_accessor";
import {
Expand Down Expand Up @@ -46,6 +48,9 @@ import { saveNowAction } from "oxalis/model/actions/save_actions";
import Toast from "libs/toast";
import messages from "messages";

const MAX_RETRY_COUNT = 5;
const RETRY_WAIT_TIME = 5000;

const isosurfacesMap: Map<number, ThreeDMap<boolean>> = new Map();
const cubeSize = [256, 256, 256];
const modifiedCells: Set<number> = new Set();
Expand Down Expand Up @@ -207,9 +212,13 @@ function* loadIsosurfaceWithNeighbors(
seedPosition != null ? seedPosition : yield* select(state => getFlooredPosition(state.flycam));
const clippedPosition = clipPositionToCubeBoundary(position, zoomStep, resolutionInfo);
let positionsToRequest = [clippedPosition];
if (seedPosition) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grittaweisheit do you know why addIsosurfaceAction is only dispatched if there is a seedPosition ? 1) The used position is set to the current annotation position and used as a seed if seedPosition == null, so it should be fine to use that here, too? 2) I'm not really sure in which cases seedPosition is null anyway. The signature states seedPosition: ?Vector, but all callers seem to pass it if I'm not missing anything?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is because ensureSuitableIsosurface is also called at every flycam movement, but we don't really want to add an isosurface then. And since there is no seed position in that case, that is the way I decided whether to dispatch the addIsosurfaceAction or not.

Copy link
Member Author

@philippotto philippotto Jan 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense. Good point with the flycam-movement listener! I missed that. However, the "if seed position is null, we know it was caused by a flycam movement"-logic is a bit too hidden for me. I just changed the code to simply dispatch addIsosurfaceAction everytime unless it already exists in the store. This is bit more intuitive and safer in my opinion.

yield* put(addIsosurfaceAction(segmentId, seedPosition));

const hasIsosurface = yield* select(state => state.isosurfaces[segmentId] != null);
if (!hasIsosurface) {
yield* put(addIsosurfaceAction(segmentId, position));
}
yield* put(startRefreshingIsosurfaceAction(segmentId));

while (positionsToRequest.length > 0) {
const currentPosition = positionsToRequest.shift();
const neighbors = yield* call(
Expand All @@ -225,6 +234,8 @@ function* loadIsosurfaceWithNeighbors(
isInitialRequest = false;
positionsToRequest = positionsToRequest.concat(neighbors);
}

yield* put(finishedRefreshingIsosurfaceAction(segmentId));
}

function hasBatchCounterExceededLimit(segmentId: number): boolean {
Expand Down Expand Up @@ -267,34 +278,46 @@ function* maybeLoadIsosurface(
const volumeTracing = yield* select(state => state.tracing.volume);
// Fetch from datastore if no volumetracing exists or if the tracing has a fallback layer.
const useDataStore = volumeTracing == null || volumeTracing.fallbackLayer != null;
const { buffer: responseBuffer, neighbors } = yield* call(
computeIsosurface,
useDataStore ? dataStoreUrl : tracingStoreUrl,
layer,
{
position: clippedPosition,
zoomStep,
segmentId,
voxelDimensions,
cubeSize,
scale,
},
);

// Check again whether the limit was exceeded, since this variable could have been
// set in the mean time by ctrl-clicking the segment to remove it
if (hasBatchCounterExceededLimit(segmentId)) {
return [];
}
const vertices = new Float32Array(responseBuffer);
if (removeExistingIsosurface) {
getSceneController().removeIsosurfaceById(segmentId);
let retryCount = 0;
while (retryCount < MAX_RETRY_COUNT) {
try {
const { buffer: responseBuffer, neighbors } = yield* call(
computeIsosurface,
useDataStore ? dataStoreUrl : tracingStoreUrl,
layer,
{
position: clippedPosition,
zoomStep,
segmentId,
voxelDimensions,
cubeSize,
scale,
},
);

// Check again whether the limit was exceeded, since this variable could have been
// set in the mean time by ctrl-clicking the segment to remove it
if (hasBatchCounterExceededLimit(segmentId)) {
return [];
}
const vertices = new Float32Array(responseBuffer);
if (removeExistingIsosurface) {
getSceneController().removeIsosurfaceById(segmentId);
}
getSceneController().addIsosurfaceFromVertices(vertices, segmentId);

return neighbors.map(neighbor =>
getNeighborPosition(clippedPosition, neighbor, zoomStep, resolutionInfo),
);
} catch (exception) {
retryCount++;
ErrorHandling.notify(exception);
console.warn("Retrying isosurface generation...");
yield* call(sleep, RETRY_WAIT_TIME * 2 ** retryCount);
}
}
getSceneController().addIsosurfaceFromVertices(vertices, segmentId);

return neighbors.map(neighbor =>
getNeighborPosition(clippedPosition, neighbor, zoomStep, resolutionInfo),
);
return [];
}

function* downloadIsosurfaceCellById(cellId: number): Saga<void> {
Expand Down Expand Up @@ -334,7 +357,11 @@ function* importIsosurfaceFromStl(action: ImportIsosurfaceFromStlAction): Saga<v
const geometry = yield* call(parseStlBuffer, buffer);
getSceneController().addIsosurfaceFromGeometry(geometry, segmentId);
yield* put(setImportingMeshStateAction(false));
yield* put(addIsosurfaceAction(segmentId, [0, 0, 0])); // TODO: use good position as seed

// TODO: Ideally, persist the seed position in the STL file. As a workaround,
// we simply use the current position as a seed position.
const seedPosition = yield* select(state => getFlooredPosition(state.flycam));
yield* put(addIsosurfaceAction(segmentId, seedPosition));
}

function* removeIsosurface(
Expand Down
6 changes: 3 additions & 3 deletions frontend/javascripts/oxalis/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -453,11 +453,11 @@ type UiInformation = {
+isRefreshingIsosurfaces: boolean,
};

type IsosurfaceInformation = {
export type IsosurfaceInformation = {|
+segmentId: number,
+seedPosition: Vector3,
+isLoading: boolean,
};
|};

export type OxalisState = {|
+datasetConfiguration: DatasetConfiguration,
Expand All @@ -471,7 +471,7 @@ export type OxalisState = {|
+viewModeData: ViewModeData,
+activeUser: ?APIUser,
+uiInformation: UiInformation,
+isosurfaces: { [segmentId: string]: IsosurfaceInformation },
+isosurfaces: { [segmentId: number]: IsosurfaceInformation },
|};

const sagaMiddleware = createSagaMiddleware();
Expand Down
9 changes: 5 additions & 4 deletions frontend/javascripts/oxalis/view/right-menu/meshes_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import _ from "lodash";

import type { ExtractReturn } from "libs/type_helpers";
import type { MeshMetaData, RemoteMeshMetaData } from "types/api_flow_types";
import type { OxalisState } from "oxalis/store";
import type { OxalisState, IsosurfaceInformation } from "oxalis/store";
import Store from "oxalis/store";
import Model from "oxalis/model";
import type { Vector3 } from "oxalis/constants";
Expand Down Expand Up @@ -137,7 +137,7 @@ type StateProps = {|
|};
type DispatchProps = ExtractReturn<typeof mapDispatchToProps>;

type Props = { ...OwnProps, ...DispatchProps, ...StateProps };
type Props = {| ...OwnProps, ...DispatchProps, ...StateProps |};

const getCheckboxStyle = isLoaded =>
isLoaded
Expand Down Expand Up @@ -263,10 +263,11 @@ class MeshesView extends React.Component<
</div>
);

const renderIsosurfaceListItem = (isosurface: Object) => {
const renderIsosurfaceListItem = (isosurface: IsosurfaceInformation) => {
const { segmentId, seedPosition, isLoading } = isosurface;
const centeredCell = getIdForPos(getPosition(this.props.flycam));
const actionVisibility = segmentId === this.state.hoveredListItem ? "visible" : "hidden";
const actionVisibility =
isLoading || segmentId === this.state.hoveredListItem ? "visible" : "hidden";
return (
<List.Item
style={{
Expand Down