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

Added the feature to Remove annotation objects in a specified range of frames #3617

Merged

Conversation

gudipudiramanakumar
Copy link
Contributor

@gudipudiramanakumar gudipudiramanakumar commented Aug 28, 2021

Issue: #33

The problem mentioned was to be able to remove all annotation objects in a given range of frames i.e. remove annotations on video from frame N till frame N+K.

Solution

  • Added a redirect to the Remove Annotations Modal to ask if the user wants to remove all the annotations or select the range of annotations to be removed in AnnotationMenuComponent

  • Added a new Modal RemoveRangeConfirmComponent that appears on clicking select range which allows the user to select the start frame and end frame of the range.

  • On selecting the range and confirming the container RemoveAnnotationsRangeContainer calls the action removeAnnotationsinRangeAsync

  • The removeAnnotationsinRangeAsync action removes the frames using the removeObjectAsync action which handles throwing errors

Note: The locked objects will not be removed

Motivation and context

This fixes Issue: #33 and the workflow for removing annotations in bulk can be improved

How has this been tested?

Tested locally by running the CVAT Application and the requested feature is working. All the Annotation Objects are being removed in the specified range of frames.

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

@bsekachev bsekachev self-assigned this Sep 23, 2021
@@ -0,0 +1,83 @@
// Copyright (C) 2020-2021 Intel Corporation
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
// Copyright (C) 2020-2021 Intel Corporation
// Copyright (C) 2021 Intel Corporation

@@ -0,0 +1,118 @@
// Copyright (C) 2020-2021 Intel Corporation
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
// Copyright (C) 2020-2021 Intel Corporation
// Copyright (C) 2021 Intel Corporation

Comment on lines 531 to 532
startFrame,
endFrame,
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need this payload? It was not changed in the function, so, I believe we do not need to pass it to reducers

Comment on lines 508 to 512
removeinrange: {
sessionInstance: any | null;
startFrame: number;
endFrame: number;
};
Copy link
Member

Choose a reason for hiding this comment

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

I do not think that the state should be global. It is only used in one Modal window.

...state,
removeinrange: {
...state.removeinrange,
sessionInstance,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this field in redux storage? It is already there. But in your case it is not necessary even to pass to removeAnnotationsinRangeAsync. Just look how fetchAnnotationsAsync is implemented.

try {

for(let frame = startFrame; frame<=endFrame; frame++){
await dispatch(changeFrameAsync(frame));
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to navigate each frame here? In this case annotation removing can fetch huge amount of time (getting frames from the server, decoding them, etc).

for(let frame = startFrame; frame<=endFrame; frame++){
await dispatch(changeFrameAsync(frame));

const states = await sessionInstance.annotations.get(frame, showAllInterpolationTracks, filters );
Copy link
Member

Choose a reason for hiding this comment

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

If showAllInterpolationTracks is true, it will remove all the tracks in the job, even if they are not presented in range.
If filters is not empty, it will not remove some objects which are not filtered according to filters.

const states = await sessionInstance.annotations.get(frame, showAllInterpolationTracks, filters );

states.forEach(async (state: any) => {
await dispatch(removeObjectAsync(sessionInstance,state,force));
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested it with undo/redo? The objects probably can be restored with undo in this case, at the same time when remove all annotations, they can't be restored.

@bsekachev
Copy link
Member

bsekachev commented Sep 23, 2021

Hi, @gudipudiramanakumar

Sorry for the long response. Generally I like this patch, but I believe, it would be better to implement removing logic on cvat-core level, instead of removing all the objects via removeObjectAsync() (I has shown in comments above why it could be bad practice, what is more it has pure performance).

For example adding addition arguments to:
await sessionInstance.annotations.clear(from, to);

And one more question is to how to deal with tracks. Shall we remove full track if it's part is in removing range? Or shall we just clear track's keyframes in this range? Maybe support both ways by additional arguments in clear() method?

}

type Props = StateToProps & DispatchToProps;
class RemoveAnnotationsRangeContainer extends React.PureComponent<Props> {
Copy link
Member

Choose a reason for hiding this comment

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

Definition of containers for components is outdated practice. Probably you can combine all the code in component file using useSelector(), useState(), and useDispatch() hooks

@bsekachev
Copy link
Member

I will close the pull request, because there is not any reply. Do not hesitate to reopen it if you plan to continue working

@bsekachev bsekachev closed this Sep 30, 2021
@gudipudiramanakumar
Copy link
Contributor Author

Hi, @bsekachev I am willing to continue my work on this issue, could you please reopen this pull request.

@bsekachev
Copy link
Member

Sure

@bsekachev bsekachev reopened this Sep 30, 2021
@nmanovic
Copy link
Contributor

nmanovic commented Oct 7, 2021

@gudipudiramanakumar , no need to keep the PR open. Just send us an updated PR as you are ready. No rush here.

@nmanovic nmanovic closed this Oct 7, 2021
Removed all the global states previously used and converted all the parameters to local state in annotation menu and remove range component.
@gudipudiramanakumar
Copy link
Contributor Author

Hey @bsekachev and @nmanovic , sorry for the delay. I started working on the issue again and completed converting the RemoveRangeConfirmComponent to hook based approach and removed all the global states related to it.

But I want to understand how the cvat-core is setup and how to add a function on the backend.

@bsekachev
Copy link
Member

@gudipudiramanakumar

how to add a function on the backend

I believe, you don't need to change backend in this feature

how the cvat-core is setup

What are your specific questions about cvat-core?

@gudipudiramanakumar
Copy link
Contributor Author

gudipudiramanakumar commented Oct 14, 2021

Hi @bsekachev,
I understood how to work with cvat-core and I got the clear method working but I observe that there are two types of removal happening in the cvat,

  • Removing the objects, shapes, tags and tracks from the lists like in "Remove Annotations" in clear method (or)
  • Only toggling the removed key to false like in "RemoveObject".

Which one should be implemented here?

@bsekachev
Copy link
Member

@gudipudiramanakumar

From my opinion - the first.
Feature removing annotations on frames range should be similar to remove all annotations, IMHO. Just only on specific frames.
The main question is what to do with tracks, where some keyframes are inside the range, some outside.

@gudipudiramanakumar
Copy link
Contributor Author

gudipudiramanakumar commented Oct 18, 2021

@bsekachev

Ok, sure currently I got that working. But for tracks I am currently deleting the track object if the start frame is in the range, do you suggest deleting only the keyframes.

And I see that you previously suggested adding additional arguments to allow both ways would be ideal, but should this also reflect on the modal as an option for users or just an additional option on the funtion.

Added arguments of startframe and endframe to clear method in annotation-collection, and also added the updating of the states with payload on removeannotationsinrangeasync action in the reducer.
Merge all the commits during the development of the feature-removeannotationsinrange
@bsekachev
Copy link
Member

I fixed all ESLint errors but one

That is probably a new rule from airbnb style preset we use.
Could you please rewrite onClickMenuWrapper as () => onClickMenuWrapper()?

@@ -0,0 +1,46 @@
"use strict";
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want to commit this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this happened by accident, how can I revert this?

Copy link
Member

Choose a reason for hiding this comment

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

Just remove it from your files and commit

Comment on lines 181 to 185
if (startframe !== undefined && endframe !== undefined) {
cache.get(session).collection.clear(startframe, endframe, delTrackKeyframesOnly);
} else {
cache.get(session).collection.clear();
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this check is extra. You call the same method and do the same check inside

delete this.objects[object.clientID];
});

this.count -= objectsRemovable.length;
Copy link
Member

Choose a reason for hiding this comment

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

this.count is used to generate clientIDs.
Do not need to decrease it if do not remove all objects, otherwise you might get several objects with the same clientID.

Copy link
Contributor Author

@gudipudiramanakumar gudipudiramanakumar Oct 20, 2021

Choose a reason for hiding this comment

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

We are removing all the given objects in range, it is also only counting if objects are being removed. Even in tracks, they are pushed in objectsRemovable only if they are being removed and not when removing keyframes.

Or are you suggesting to completely avoid this line as the garbage collector is removing the objects as you mentioned below

Copy link
Member

Choose a reason for hiding this comment

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

should I completely avoid this line as the garbage collector is removing the objects

Yes. If you look at previous clear() implementation, it just clears all the arrays and objects. We do not remove each object separately there

Comment on lines 582 to 584
objectsRemovable.forEach((object) => {
delete this.objects[object.clientID];
});
Copy link
Member

Choose a reason for hiding this comment

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

Probably we do not need to remove them explicitly. Garbage collector does it itself when there are no links to these objects anymore.


this.count -= objectsRemovable.length;

this.flush = 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 believe if remove from range, this flag should not be set to true.
If this flag is true, client sends put annotation request and all annotations are removed from the database.
If this flag is false, client sends only changes.

@@ -37,8 +37,8 @@
return result;
},

async clear(reload = false) {
const result = await PluginRegistry.apiWrapper.call(this, prototype.annotations.clear, reload);
async clear(reload = false, startframe, endframe, delTrackKeyframesOnly = true) {
Copy link
Member

Choose a reason for hiding this comment

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

Arguments with default parameters should be placed at the end.
Maybe just set startframe and endframe to null as default?

Comment on lines 510 to 511
// eslint-disable-next-line max-len
export function removeAnnotationsinRangeAsync(sessionInstance: any, startFrame: number, endFrame: number, delTrackKeyframesOnly: boolean): ThunkAction {
Copy link
Member

Choose a reason for hiding this comment

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

Do not need to add // eslint-disable-next-line max-len
Just format lines. For example this way:

export function removeAnnotationsinRangeAsync(
    sessionInstance: any, startFrame: number, endFrame: number, delTrackKeyframesOnly: boolean
): ThunkAction {

Copy link
Member

Choose a reason for hiding this comment

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

Also instead of passing sessionInstance here, just do:

const { jobInstance } = receiveAnnotationsParameters();

in the function body.

} from 'actions/annotation-actions';
import { exportActions } from 'actions/export-actions';

interface StateToProps {
annotationFormats: any;
jobInstance: any;
stopFrame: any;
Copy link
Member

Choose a reason for hiding this comment

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

Why any? It is a number

@@ -32,6 +34,8 @@ interface DispatchToProps {
loadAnnotations(job: any, loader: any, file: File): void;
showExportModal(task: any): void;
removeAnnotations(sessionInstance: any): void;
// eslint-disable-next-line max-len
Copy link
Member

Choose a reason for hiding this comment

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

Please, avoid adding these directives. Just format the line

Comment on lines 159 to 161
const removeRange = (startFrame: number, endFrame: number, delTrackKeyframesOnly:boolean) : void => {
removeAnnotationinRangeAsync(jobInstance, startFrame, endFrame, delTrackKeyframesOnly);
};
Copy link
Member

Choose a reason for hiding this comment

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

Do not need to create one more wrapper to pass jobInstance. jobInstance can be fetched in ThunkAction body. Please, refer my comment above.

@@ -74,6 +82,11 @@ function mapDispatchToProps(dispatch: any): DispatchToProps {
removeAnnotations(sessionInstance: any): void {
dispatch(removeAnnotationsAsync(sessionInstance));
},
// eslint-disable-next-line max-len
removeAnnotationinRangeAsync(sessionInstance:any, startnumber: number, endnumber: number, delTrackKeyframesOnly:boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

Name it just:

removeAnnotationinRange

@gudipudiramanakumar
Copy link
Contributor Author

gudipudiramanakumar commented Oct 20, 2021

I fixed all ESLint errors but one

That is probably a new rule from airbnb style preset we use. Could you please rewrite onClickMenuWrapper as () => onClickMenuWrapper()?

Hi @bsekachev
Added the onClickMenuWrapper in onClick according to the new rule with passing the parameters like (params: MenuInfo) => onClickMenuWrapper(params). Could you review all the changes?

Changed all the suggested changes and also removed unnecessary files in dist.
Removed unnecessary explicit removals in objects and additional wrappers.
Fixed the mistake of wrong variable name.
Additional ESLint Issue fixed
@@ -553,7 +553,35 @@
return groupIdx;
}

clear() {
clear(startframe, endframe, delTrackKeyframesOnly) {
let objectsRemovable = [];
Copy link
Member

Choose a reason for hiding this comment

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

Now it is unused variable

Comment on lines 120 to 122
onCancel: () => {
toggleOpenRemoveRangeComponent(!openRemoveRangeComponent);
},
Copy link
Member

Choose a reason for hiding this comment

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

I do not think it is the best solution. Cancel button is not obvious to do action you programmed.

'It will stay on the server till you save the job. Continue?',
className: 'cvat-modal-confirm-remove-annotation',
onOk: () => {
onClickMenu(params);
Modal.confirm({
Copy link
Member

@bsekachev bsekachev Oct 21, 2021

Choose a reason for hiding this comment

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

I would suggest following:

pseudocode

let removeFrom;
let removeUpTo;
let removeOnlyKeyframes = false;
Modal.confirm({
    content: (
        <div>  // of course, need to format content properly  
            <Text>message</Text> // of course, need to format content properly 
            <Text>Leave the inputs below empty to remove all the annotations from this job</Text>
            <Text>From:</Text><InputNumber onChange={(value) => removeFrom = value} />
            <Text>To:</Text><InputNumbe onChange={(value) => removeUpTo= value} />
            <Tooltip title='Applicable if remove annotations in range'>
                <Checkbox onClick={(checked) => removeOnlyKeyframes = checked}>Delete only keyframes for tracks</Checkbox>
            </Tooltip>
        </div>
    )

    onOk: () => removeAnnotations(removeFrom, removeUpTo, )
})

Copy link
Member

Choose a reason for hiding this comment

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

Here you do not need remove-range-confirm.tsx Component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll try this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you also suggest clubbing the removeAnnotationsAsync and removeAnnotationsinRangeAsync actions as one action

Copy link
Member

Choose a reason for hiding this comment

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

So, I think yes. They are very similar each other.

Changed the approach of removeAnnotations modal so that it could match the implementation of all the other components
@gudipudiramanakumar
Copy link
Contributor Author

Hi @bsekachev
I worked on the suggested changes, I added a Collapse from Ant Design on top of the suggested approach for hiding the inputs when not needed. Please do check the commit and do suggest if anything else needs to be updated.

@gudipudiramanakumar
Copy link
Contributor Author

Hi @bsekachev
I did not understand what has failed in the workflow run, can you suggest what to be done.

@bsekachev
Copy link
Member

Hi @gudipudiramanakumar

Don't worry about CI now. Looks like the same test failed in several PRs.

@@ -80,14 +89,52 @@ export default function AnnotationMenuComponent(props: Props): JSX.Element {
}

if (params.key === Actions.REMOVE_ANNO) {
let removeFrom: any;
let removeUpTo: any;
let removeOnlyKeyframes: any = false;
Copy link
Member

Choose a reason for hiding this comment

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

Please, use: boolean instead of any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did change the numbers but ESLint is throwing this error for boolean, it is suggesting to keep

let removeOnlyKeyframes = false;

Type boolean trivially inferred from a boolean literal, remove type annotation. eslint@typescript-eslint/no-inferrable-types

Copy link
Member

Choose a reason for hiding this comment

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

Okay, than just: let removeOnlyKeyframes = false;

Comment on lines 92 to 93
let removeFrom: any;
let removeUpTo: any;
Copy link
Member

Choose a reason for hiding this comment

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

Please, use number instead of any.

@bsekachev
Copy link
Member

Thank you for this great job!
Just a couple of comments and please, update versions for cvat-core and cvat-ui:

cd cvat-core && npm version minor
cd ../cvat-ui && npm version minor

And add a line to CHANGELOG.md file (added section):

  • The feature to remove annotations in a specified range of frames (<https://github.com/openvinotoolkit/cvat/pull/3617>)

Fixed type annotations in the annotation-menu component for remove annotations, and updated cvat-ui and cvat-core npm versions.
@gudipudiramanakumar
Copy link
Contributor Author

Hi @bsekachev ,
I updated the mentioned changes and added the commits. Please have a look.

@bsekachev bsekachev merged commit 78158cb into cvat-ai:develop Oct 26, 2021
@bsekachev
Copy link
Member

Great, thank you again!

@gudipudiramanakumar
Copy link
Contributor Author

Thanks a lot for guiding me all the way, this was my first contribution and I am very happy to have a chance to work on this project and the issue. Thank you very much.

@bsekachev
Copy link
Member

@aschernov @TOsmanov

Hi guys, could you please update documentation after this patch will be available?
I suppose need just to update the screenshot and add one sentence.

@TOsmanov
Copy link
Contributor

@aschernov @TOsmanov

Hi guys, could you please update documentation after this patch will be available? I suppose need just to update the screenshot and add one sentence.

@bsekachev Hi, yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants