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.

Sign up for GitHub

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

Stop editing when n key pressed #7922

Merged
merged 11 commits into from
Jun 19, 2024
Merged

Stop editing when n key pressed #7922

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d338710
stop drawing when n key pressed
KrishavRajSingh May 21, 2024
6b526cc
added changelog.md
KrishavRajSingh May 21, 2024
a9567e9
catched n pressing event in cvat-ui
KrishavRajSingh May 28, 2024
f5f907f
merged conflict
KrishavRajSingh Jun 1, 2024
90a19a9
Merge branch 'develop' into fix/endPolyline
bsekachev Jun 3, 2024
04ab539
Updated changelog
bsekachev Jun 3, 2024
4708548
Merge branch 'cvat-ai:develop' into fix/endPolyline
KrishavRajSingh Jun 9, 2024
99e7ed2
fixed polygon editing
KrishavRajSingh Jun 9, 2024
8a72123
Merge branch 'develop' into fix/endPolyline
bsekachev Jun 12, 2024
2e09878
Merge branch 'cvat-ai:develop' into fix/endPolyline
KrishavRajSingh Jun 15, 2024
bcb9d79
fixed polygon issue
KrishavRajSingh Jun 15, 2024
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: 4 additions & 0 deletions changelog.d/20240521_132410_krishavrajsingh_endPolyline.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
### Added

- Polyline editing may be finished using corresponding shortcut
(<https://github.com/cvat-ai/cvat/pull/7922>)
5 changes: 3 additions & 2 deletions cvat-canvas/src/typescript/canvas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
CanvasModel, CanvasModelImpl, RectDrawingMethod,
CuboidDrawingMethod, Configuration, Geometry, Mode,
HighlightSeverity as _HighlightSeverity, CanvasHint as _CanvasHint,
PolyEditData,
} from './canvasModel';
import { Master } from './master';
import { CanvasController, CanvasControllerImpl } from './canvasController';
Expand All @@ -35,7 +36,7 @@ interface Canvas {

interact(interactionData: InteractionData): void;
draw(drawData: DrawData): void;
edit(editData: MasksEditData): void;
edit(editData: MasksEditData | PolyEditData): void;
group(groupData: GroupData): void;
join(joinData: JoinData): void;
slice(sliceData: SliceData): void;
Expand Down Expand Up @@ -137,7 +138,7 @@ class CanvasImpl implements Canvas {
this.model.draw(drawData);
}

public edit(editData: MasksEditData): void {
public edit(editData: MasksEditData | PolyEditData): void {
this.model.edit(editData);
}

Expand Down
9 changes: 5 additions & 4 deletions cvat-canvas/src/typescript/canvasController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
Configuration,
MasksEditData,
HighlightedElements,
PolyEditData,
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider specifying a more precise type than any for objects in CanvasController.

- readonly objects: any[];
+ readonly objects: ObjectType[]; // Replace 'ObjectType' with the actual type used for objects.

Committable suggestion was skipped due low confidence.

} from './canvasModel';

export interface CanvasController {
Expand All @@ -30,7 +31,7 @@ export interface CanvasController {
readonly activeElement: ActiveElement;
readonly highlightedElements: HighlightedElements;
readonly drawData: DrawData;
readonly editData: MasksEditData;
readonly editData: MasksEditData | PolyEditData;
readonly interactionData: InteractionData;
readonly mergeData: MergeData;
readonly splitData: SplitData;
Expand All @@ -44,7 +45,7 @@ export interface CanvasController {

zoom(x: number, y: number, direction: number): void;
draw(drawData: DrawData): void;
edit(editData: MasksEditData): void;
edit(editData: MasksEditData | PolyEditData): void;
enableDrag(x: number, y: number): void;
drag(x: number, y: number): void;
disableDrag(): void;
Expand Down Expand Up @@ -96,7 +97,7 @@ export class CanvasControllerImpl implements CanvasController {
this.model.draw(drawData);
}

public edit(editData: MasksEditData): void {
public edit(editData: MasksEditData | PolyEditData): void {
this.model.edit(editData);
}

Expand Down Expand Up @@ -136,7 +137,7 @@ export class CanvasControllerImpl implements CanvasController {
return this.model.drawData;
}

public get editData(): MasksEditData {
public get editData(): MasksEditData | PolyEditData {
return this.model.editData;
}

Expand Down
14 changes: 7 additions & 7 deletions cvat-canvas/src/typescript/canvasModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,8 @@ export interface InteractionResult {

export interface PolyEditData {
enabled: boolean;
state: any;
pointID: number;
state?: any;
pointID?: number;
Comment on lines +150 to +151
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor to avoid using any type for better type safety.

-    state?: any;
+    state?: StateType; // Define StateType appropriately based on the expected structure

Committable suggestion was skipped due low confidence.

}

export interface MasksEditData {
Expand Down Expand Up @@ -249,7 +249,7 @@ export interface CanvasModel {
readonly activeElement: ActiveElement;
readonly highlightedElements: HighlightedElements;
readonly drawData: DrawData;
readonly editData: MasksEditData;
readonly editData: MasksEditData | PolyEditData;
readonly interactionData: InteractionData;
readonly mergeData: MergeData;
readonly splitData: SplitData;
Expand All @@ -275,7 +275,7 @@ export interface CanvasModel {
grid(stepX: number, stepY: number): void;

draw(drawData: DrawData): void;
edit(editData: MasksEditData): void;
edit(editData: MasksEditData | PolyEditData): void;
group(groupData: GroupData): void;
join(joinData: JoinData): void;
slice(sliceData: SliceData): void;
Expand Down Expand Up @@ -369,7 +369,7 @@ export class CanvasModelImpl extends MasterImpl implements CanvasModel {
fittedScale: number;
zLayer: number | null;
drawData: DrawData;
editData: MasksEditData;
editData: MasksEditData | PolyEditData;
interactionData: InteractionData;
mergeData: MergeData;
groupData: GroupData;
Expand Down Expand Up @@ -771,7 +771,7 @@ export class CanvasModelImpl extends MasterImpl implements CanvasModel {
this.notify(UpdateReasons.DRAW);
}

public edit(editData: MasksEditData): void {
public edit(editData: MasksEditData | PolyEditData): void {
if (![Mode.IDLE, Mode.EDIT].includes(this.data.mode)) {
throw Error(`Canvas is busy. Action: ${this.data.mode}`);
}
Expand Down Expand Up @@ -1074,7 +1074,7 @@ export class CanvasModelImpl extends MasterImpl implements CanvasModel {
return { ...this.data.drawData };
}

public get editData(): MasksEditData {
public get editData(): MasksEditData | PolyEditData {
return { ...this.data.editData };
}

Expand Down
2 changes: 2 additions & 0 deletions cvat-canvas/src/typescript/canvasView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1641,6 +1641,8 @@ export class CanvasViewImpl implements CanvasView, Listener {
this.masksHandler.edit(data);
} else if (this.masksHandler.enabled) {
this.masksHandler.edit(data);
} else if (this.editHandler.enabled) {
this.editHandler.edit(data);
}
} else if (reason === UpdateReasons.INTERACT) {
const data: InteractionData = this.controller.interactionData;
Expand Down
39 changes: 24 additions & 15 deletions cvat-canvas/src/typescript/editHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export interface EditHandler {
transform(geometry: Geometry): void;
configurate(configuration: Configuration): void;
cancel(): void;
enabled: boolean;
}

export class EditHandlerImpl implements EditHandler {
Expand All @@ -31,9 +32,9 @@ export class EditHandlerImpl implements EditHandler {
private autobordersEnabled: boolean;
private intelligentCutEnabled: boolean;
private outlinedBorders: string;
private isEditing: boolean;

private setupTrailingPoint(circle: SVG.Circle): void {
const head = this.editedShape.attr('points').split(' ').slice(0, this.editData.pointID).join(' ');
circle.on('mouseenter', (): void => {
circle.attr({
'stroke-width': consts.POINTS_SELECTED_STROKE_WIDTH / this.geometry.scale,
Expand All @@ -46,22 +47,11 @@ export class EditHandlerImpl implements EditHandler {
});
});

const minimumPoints = 2;
circle.on('mousedown', (e: MouseEvent): void => {
if (e.button !== 0) return;
const { offset } = this.geometry;
const stringifiedPoints = `${head} ${this.editLine.node.getAttribute('points').slice(0, -2)}`;
const points = pointsToNumberArray(stringifiedPoints)
.slice(0, -2)
.map((coord: number): number => coord - offset);

if (points.length >= minimumPoints * 2) {
const { state } = this.editData;
this.edit({
enabled: false,
});
this.onEditDone(state, points);
}
this.edit({
enabled: false,
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.edit({
enabled: false,
});
this.edit({ enabled: false });

});
}

Expand Down Expand Up @@ -380,6 +370,19 @@ export class EditHandlerImpl implements EditHandler {
}

private closeEditing(): void {
if (this.isEditing) {
const { offset } = this.geometry;
const head = this.editedShape.attr('points').split(' ').slice(0, this.editData.pointID).join(' ');
const stringifiedPoints = `${head} ${this.editLine.node.getAttribute('points').slice(0, -2)}`;
const points = pointsToNumberArray(stringifiedPoints)
.slice(0, -2)
.map((coord: number): number => coord - offset);
if (points.length >= 2 * 2) { // minimumPoints * 2
const { state } = this.editData;
this.onEditDone(state, points);
}
}
this.isEditing = false;
Copy link
Member

Choose a reason for hiding this comment

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

  • I believe we should set this.isEditing = false in release method
  • I believe we should set this.isEditing = true in initEditing method

Copy link
Member

Choose a reason for hiding this comment

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

Probably release shall be called inside the condition. Otherwise it leads to the issue, I described in the pull request with polygons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Release method needs to be called whenever a polyline is edited, so i think it should be outside.

As for the issue, I fixed it by cheking the shapeType of the editHandler and pass {enabled: false} only if the shapetype is polyline in canvasview.ts. It shall work fine now.

this.release();
Copy link
Member

Choose a reason for hiding this comment

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

Shall we put these two lines under the condition?

Copy link
Contributor Author

@KrishavRajSingh KrishavRajSingh Jun 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since the polygon editing was affected, i am thinking to add checktype for shape, something like this
`

  •    if (this.isEditing && this.editData.state.shapeType === 'polyline') {
     ...
  } else {
      this.onEditDone(null, null);
  }
  this.isEditing = false;
  this.release();

`
So I think I shall keep it outside.
I am open to suggestions.

}

Expand All @@ -400,12 +403,14 @@ export class EditHandlerImpl implements EditHandler {
this.editLine = null;
this.geometry = null;
this.clones = [];
this.isEditing = false;
}

public edit(editData: any): void {
if (editData.enabled) {
if (editData.state.shapeType !== 'rectangle') {
this.editData = editData;
this.isEditing = true;
Copy link
Member

Choose a reason for hiding this comment

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

I will suggest:

  1. Update condition as: ['polygon', 'polyline', 'points'].includes(editData.state.shapeType) just to make code clearer as this handler only works with these types.
  2. Set isEditing after this.initEditing as this is more reliable (in case if this.initEditing throws an exception)

this.initEditing();
} else {
this.cancel();
Expand All @@ -421,6 +426,10 @@ export class EditHandlerImpl implements EditHandler {
this.onEditDone(null, null);
}

get enabled(): boolean {
return this.isEditing;
}

public configurate(configuration: Configuration): void {
this.autobordersEnabled = configuration.autoborders;
this.outlinedBorders = configuration.outlinedBorders || 'black';
Expand Down
2 changes: 1 addition & 1 deletion ...src/components/annotation-page/standard-workspace/controls-side-bar/controls-side-bar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ export default function ControlsSideBarComponent(props: Props): JSX.Element {
if (!drawing) {
if (editing) {
// users probably will press N as they are used to do when they want to finish editing
// in this case, if a mask is being edited we probably want to finish editing first
// in this case, if a mask or polyline is being edited we probably want to finish editing first
canvasInstance.edit({ enabled: false });
return;
}
Expand Down
Loading