Skip to content

Commit

Permalink
Miscellaneous improvements to the proofreading tool (#6477)
Browse files Browse the repository at this point in the history
* fix race condition in conflict-checking code by re-reading current version from store

* don't try to load agglomerate skeleton if user selects a node in proofreading tool

* remove [Object object] from error toast

* remove console.log

* use base mapping name of editable mapping in mapping UI if possible

* update changelog

* fix linting
  • Loading branch information
philippotto authored Sep 21, 2022
1 parent 3fffc69 commit b58497e
Show file tree
Hide file tree
Showing 7 changed files with 58 additions and 28 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- The proofreading tool now supports merging and splitting (via min-cut) agglomerates by rightclicking a segment (and not a node). Note that there still has to be an active node so that both partners of the operation are defined. [#6464](https://github.com/scalableminds/webknossos/pull/6464)

### Changed
- Selecting a node with the proofreading tool won't have any side effects anymore. Previous versions could load additional agglomerate skeletons in certain scenarios which could be confusing. [#6477](https://github.com/scalableminds/webknossos/pull/6477)
- Sharing links are shortened by default. Within the sharing modal, this shortening behavior can be disabled. [#6461](https://github.com/scalableminds/webknossos/pull/6461)
- Removed optional "resolution" parameter from /datasets/:organizationName/:dataSetName/layers/:dataLayerName/data route. Use mag instead. [#6479](https://github.com/scalableminds/webknossos/pull/6479)

Expand All @@ -29,6 +30,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Fixed importing of remote datastore (e.g., zarr) when datastore is set up separately. [#6462](https://github.com/scalableminds/webknossos/pull/6462)
- Fixed a crash which could happen when using the "Automatically clip histogram" feature in certain scenarios. [#6433](https://github.com/scalableminds/webknossos/pull/6433)
- Fixed loading agglomeate skeletons for agglomerate ids larger than 2^31. [#6472](https://github.com/scalableminds/webknossos/pull/6472)
- Fixed bug which could lead to conflict-warnings even though there weren't any. [#6477](https://github.com/scalableminds/webknossos/pull/6477)

### Removed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,13 @@ import {
handleResizingBoundingBox,
highlightAndSetCursorOnHoveredBoundingBox,
} from "oxalis/controller/combinations/bounding_box_handlers";
import Store, { SkeletonTracing } from "oxalis/store";
import Store from "oxalis/store";
import * as Utils from "libs/utils";
import * as VolumeHandlers from "oxalis/controller/combinations/volume_handlers";
import { document } from "libs/window";
import api from "oxalis/api/internal_api";
import { proofreadAtPosition } from "oxalis/model/actions/proofread_actions";
import { calculateGlobalPos } from "oxalis/model/accessors/view_mode_accessor";
import {
getNodeAndTree,
getSkeletonTracing,
} from "oxalis/model/accessors/skeletontracing_accessor";

export type ActionDescriptor = {
leftClick?: string;
Expand Down Expand Up @@ -640,25 +636,18 @@ export class ProofreadTool {
isTouch: boolean,
) {
const didSelectNode = SkeletonHandlers.handleSelectNode(planeView, pos, plane, isTouch);
if (didSelectNode) {
// Don't do anything else
return;
}

let globalPosition;
if (plane === OrthoViews.TDView) {
// In the 3D viewport the click position cannot be uniquely determined, because the position on the
// third axis is ambiguous. However, if the user clicked on a node, we can determine the position
// by looking up the position of the selected node.
if (didSelectNode) {
getSkeletonTracing(Store.getState().tracing).map((skeletonTracing: SkeletonTracing) =>
getNodeAndTree(skeletonTracing).map(([_activeTree, activeNode]) => {
globalPosition = activeNode.position;
}),
);
}
} else {
globalPosition = calculateGlobalPos(Store.getState(), pos);
// The click position cannot be mapped to a 3D coordinate in the
// 3D viewport (unless a node was clicked which is already handled above).
return;
}

if (globalPosition == null) return;

const globalPosition = calculateGlobalPos(Store.getState(), pos);
Store.dispatch(proofreadAtPosition(globalPosition));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,16 @@ export function isMappingActivationAllowed(
return mappingName === volumeTracing.mappingName;
}

export function getEditableMappingForVolumeTracingId(
state: OxalisState,
tracingId: string | null | undefined,
) {
if (tracingId == null) {
return null;
}
return state.tracing.mappings.find((mapping) => mapping.tracingId === tracingId);
}

export function getLastLabelAction(volumeTracing: VolumeTracing): LabelAction | undefined {
return volumeTracing.lastLabelActions[0];
}
Expand Down
23 changes: 20 additions & 3 deletions frontend/javascripts/oxalis/model/sagas/save_saga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1125,14 +1125,29 @@ function* watchForSaveConflicts() {
]);

for (const tracing of tracings) {
const version = yield* call(
const versionOnServer = yield* call(
getNewestVersionForTracing,
tracingStoreUrl,
tracing.tracingId,
tracing.type,
);

if (version > tracing.version) {
// Read the tracing version again from the store, since the
// old reference to tracing might be outdated now due to the
// immutability.
const versionOnClient = yield* select((state) => {
if (tracing.type === "volume") {
return getVolumeTracingById(state.tracing, tracing.tracingId).version;
}
const { skeleton } = state.tracing;
if (skeleton == null) {
throw new Error("Skeleton must exist at this point.");
}
return skeleton.version;
});

const toastKey = `save_conflicts_warning_${tracing.tracingId}`;
if (versionOnServer > versionOnClient) {
// The latest version on the server is greater than the most-recently
// stored version.

Expand All @@ -1153,8 +1168,10 @@ function* watchForSaveConflicts() {
}
Toast.warning(msg, {
sticky: true,
key: "save_conflicts_warning",
key: toastKey,
});
} else {
Toast.close(toastKey);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ function useUpdatePrivateLink(annotationId: string) {
},
// If the mutation fails, use the context returned from onMutate to roll back
onError: (err, _updatedLinkItem, context) => {
Toast.error(`Could not update link. ${err}`);
Toast.error("Could not update link.");
if (context) {
queryClient.setQueryData(mutationKey, context.previousLinks);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import debounceRender from "react-debounce-render";
import type { APIDataset, APISegmentationLayer } from "types/api_flow_types";
import type { OrthoView, Vector3, Vector4 } from "oxalis/constants";
import { MappingStatusEnum } from "oxalis/constants";
import type { OxalisState, Mapping, MappingType } from "oxalis/store";
import type { OxalisState, Mapping, MappingType, EditableMapping } from "oxalis/store";
import { getMappingsForDatasetLayer, getAgglomeratesForDatasetLayer } from "admin/admin_rest_api";
import { getPosition } from "oxalis/model/accessors/flycam_accessor";
import {
Expand All @@ -22,7 +22,10 @@ import {
import { SwitchSetting } from "oxalis/view/components/setting_input_views";
import * as Utils from "libs/utils";
import { jsConvertCellIdToHSLA } from "oxalis/shaders/segmentation.glsl";
import { hasEditableMapping } from "oxalis/model/accessors/volumetracing_accessor";
import {
getEditableMappingForVolumeTracingId,
hasEditableMapping,
} from "oxalis/model/accessors/volumetracing_accessor";
const { Option, OptGroup } = Select;

type OwnProps = {
Expand All @@ -38,6 +41,7 @@ type StateProps = {
hideUnmappedIds: boolean | null | undefined;
mappingType: MappingType;
mappingColors: Array<number> | null | undefined;
editableMapping: EditableMapping | null | undefined;
setMappingEnabled: (arg0: string, arg1: boolean) => void;
setHideUnmappedIds: (arg0: string, arg1: boolean) => void;
setAvailableMappingsForLayer: (arg0: string, arg1: Array<string>, arg2: Array<string>) => void;
Expand Down Expand Up @@ -182,7 +186,10 @@ class MappingSettingsView extends React.Component<Props, State> {
const selectValueProp =
this.props.mappingName != null
? {
value: this.props.mappingName,
value:
this.props.editableMapping != null
? `${this.props.editableMapping.baseMappingName} (${this.props.mappingName})`
: this.props.mappingName,
}
: {};

Expand Down Expand Up @@ -280,6 +287,9 @@ function mapStateToProps(state: OxalisState, ownProps: OwnProps) {
state.temporaryConfiguration.activeMappingByLayer,
ownProps.layerName,
);
const segmentationLayer = getSegmentationLayerByName(state.dataset, ownProps.layerName);
const editableMapping = getEditableMappingForVolumeTracingId(state, segmentationLayer.tracingId);

return {
dataset: state.dataset,
position: getPosition(state.flycam),
Expand All @@ -290,9 +300,10 @@ function mapStateToProps(state: OxalisState, ownProps: OwnProps) {
mappingType: activeMappingInfo.mappingType,
mappingColors: activeMappingInfo.mappingColors,
activeViewport: state.viewModeData.plane.activeViewport,
segmentationLayer: getSegmentationLayerByName(state.dataset, ownProps.layerName),
segmentationLayer,
isMergerModeEnabled: state.temporaryConfiguration.isMergerModeEnabled,
allowUpdate: state.tracing.restrictions.allowUpdate,
editableMapping,
isEditableMappingActive: hasEditableMapping(state, ownProps.layerName),
};
}
Expand Down
1 change: 1 addition & 0 deletions frontend/javascripts/types/api_flow_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,7 @@ export type ServerEditableMapping = {
createdTimestamp: number;
version: number;
mappingName: string;
baseMappingName: string;
// The id of the volume tracing the editable mapping belongs to
tracingId: string;
};
Expand Down

0 comments on commit b58497e

Please sign in to comment.