-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: UI for tracking #1884
feat: UI for tracking #1884
Conversation
Pull Request Test Coverage Report for Build 6519
💛 - Coveralls |
@bsekachev , could you please look at the PR? Probably you want to merge it with minimal changes from @savan77 and after that polish on our side if it is necessary. |
@nmanovic Sure, will do it. |
const trackTypeOptions = [ | ||
{ | ||
label: "OpenCV Boosting", | ||
value: "Boosting", | ||
}, | ||
{ | ||
label: "OpenCV MIL", | ||
value: "MIL", | ||
}, | ||
{ | ||
label: "OpenCV KCF", | ||
value: "KCF", | ||
}, | ||
{ | ||
label: "OpenCV CSRT", | ||
value: "CSRT", | ||
}, | ||
{ | ||
label: "OpenCV MedianFlow", | ||
value: "MEDIANFLOW", | ||
}, | ||
{ | ||
label: "OpenCV TLD", | ||
value: "TLD", | ||
}, | ||
{ | ||
label: "OpenCV MOSSE", | ||
value: "MOSSE", | ||
}, | ||
{ | ||
label: "GOTURN", | ||
value: "GOTURN", | ||
}, | ||
] |
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.
Let's avoid hardcoding of tracker types on client side. I would suggest two options here:
- Get these types from the server
- Implement UI only for one of tracker as the initial version
In the future we want to be able to work with different trackers, implemented as serverless functions.
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.
That server side function is not implemented yet but I think we should be able to add it easily as we move forward.
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.
It should be implemented before we merge this PR, since our users will have questions about new controls in UI, that are useless for them right now, because our server doesn't support tracking yet.
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.
@bsekachev As per our discussion with @nmanovic , this PR was supposed to be for UI only since you guys are implementing a serverless function for tracking. We can update UI assuming there is an API, let's say get_trackers_list
, and make appropriate changes in the front-end. I understand we can't merge it until we have the backend. I suggest we merge this PR and PR for the serverless tracking function together, once we have it.
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.
@savan77 Sure, this PR is for UI only. I mean UI shouldn't have hardcoded tracker types and display controls related with tracking by default (at this moment, till we haven't implemented server tracking). I think making temporary stubs (roughly speaking: const supportTracking = async (): boolean => false
and const getTrackers = async (): any[] => [])
is a suitable option). You do not need even make real server requests.
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.
@bsekachev can you check now? @TheNaman047 pushed a change which checks for at least one tracker to show the tracker settings.
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.
@savan77 Sure, I will. But as I can see, we still have hardcoded trackers on UI (from OpenCV). Is it outdated code?
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.
@bsekachev that's right. @TheNaman047 As we discussed, we should not show any trackers by default. We need to show tracker settings only if we get positive response from that API call. And show that trackers in the list, not the hard-coded once.
Please correct me if I am wrong. @bsekachev
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.
Yes, there were two ideas in this thread above. The first is what you mentioned and the second is do not hardcode trackers on client side. UI should get these types from the server. Right now one of trackers is implemented as a nuclio serverless function and we might use it in the implementation.
I understand if it isn't your priority, on our side I am ready to modify the PR next week
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.
Thanks @bsekachev , that would be great since you have some ideas about this new serverless function.
...rc/components/annotation-page/standard-workspace/controls-side-bar/track-popover-control.tsx
Outdated
Show resolved
Hide resolved
cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-item.tsx
Outdated
Show resolved
Hide resolved
cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-item.tsx
Show resolved
Hide resolved
Hi, thx for the PR, really appreciate it. Please, contact me if you have any questions about our code |
Thanks @bsekachev @TheNaman047 can you take a look at these comments and make appropriate changes? |
Yes, already on it.
…On Mon, 13 Jul, 2020, 22:04 Savan Visalpara, ***@***.***> wrote:
Thanks @bsekachev <https://github.com/bsekachev>
@TheNaman047 <https://github.com/TheNaman047> can you take a look at
these comments and make appropriate changes?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1884 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGDGPYW5KAWWCUHSGGGTUCTR3MZRRANCNFSM4OW2RCSA>
.
|
Hi, what are updates on the PR? |
Hi @bsekachev, Thanks for reviewing the code. Have corrected the recommended changes. |
Thx, what have you decided about dinamic controls displaying? This issue is quite critical IMHO. |
@savan77 , @TheNaman047 , @rushtehrani , I have added support for trackers on server: #1988. It is your turn now. Could you please help to finalise the PR and check that it works with SiamMask from the mentioned PR? I'm happy to help and answer questions. |
@nmanovic Thanks. We have been busy with one urgent piece. Anyway, I think PR is good to go except for one change that @bsekachev requested. I will ask @TheNaman047 to take a look as soon as possible. |
We have made required changes. I will be testing and pushing them soon. |
@savan77 , it is great to hear! Do you have any estimation when it will be ready to merge? |
@nmanovic - @TheNaman047 has pushed required changes. |
@rushtehrani Thanks, just corrected those typos. |
@savan77 , I see at least one conflict. Could you please resolve it? @bsekachev , could you please look at the current patch and comment? |
@savan77 , as I discussed with Rush and Don, my team is going to implement the feature slightly different. Also the PR isn't ready and we have decided to close it. In any case thanks for the contribution. It gave us a lot of ideas and a ready to use prototype. |
Hi @nmanovic , thanks for your update, I was recently looking at this branch because the truly object tracking function by AI models would largely help accelerate the annotation speed for our project. Today I saw you closed it, is that meant this function is still ongoing, haven't finished yet, probably your team would work more on the new design later, right? |
@mhe-wyze , it is right. Recently we have added support for the feature as a serverless function. @bsekachev is going to add UI for tracking in coming months (probably significantly early). |
This PR adds UI for tracking.
Motivation and context
The older PR for tracking was using older UI and wasn't compatible with new UI. This PR ported UI to the new UI. This is similar to what we had earlier. The only change is for Stop frame (Track Until) we are using a specific frame number rather than the Next Keyframe, etc since it is easier for users to say "track until next 50 or so frames". But its still there in the settings UI in case you want to use it.
Below are some screenshots.
How has this been tested?
It has been tested locally as well as on Onepanel platform.
Checklist
x
] I submit my changes into thedevelop
branchcvat-core, cvat-data and cvat-ui)
I will update the documentation once it has been accepted.
License
Feel free to contact the maintainers if that's a concern.
CC: @rushtehrani @nmanovic @TheNaman047