-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(annotator): 🚀 introduce circle annotator #372
Conversation
Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
Preview in video (used tracking example) https://drive.google.com/file/d/1e-6JT7xUiYjH6tsDY27siVvjEJoGNSgL/view?usp=sharing |
cc @hardikdava can you take a look |
@onuralpszr We already implemented |
@hardikdava Ellipse stay under the object. But circle I made is around object like Box so they are different but I forgot about that PR. |
Hi @onuralpszr and @hardikdava 👋🏻! Thanks a lot for your patience. I'm trying to catch up, but it will probably take a few more days before I'm fully in rhythm. As for the PR, @onuralpszr, I guess you got motivated by this issue? @hardikdava is right; we have a separate PR with different annotators, but I think we need to add them one by one. Going for all of them at the same time is hard to review. So, I'd like to go forward with this PR. I'll do the review. |
Yes |
I also reviewed our new annotator system, if we want to merge our big annotator change PR, I can move this one to over there too. I can also handle "triangle" part too |
@@ -147,6 +133,128 @@ def annotate( | |||
return scene | |||
|
|||
|
|||
class CircleAnnotator(BaseAnnotator): |
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.
I think that CircleAnnotator
should only draw circles.
In the next PR, we should add TextAnnotator
responsible for drawing labels with background. TextAnnotator
could be used in combination with CircleAnnotator
, TriangleAnnotator
, EllipseAnnotator
, and so on.
@onuralpszr please drop the whole text drawing logic, and keep only the circles drawing.
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.
@SkalskiP I will do changes but can we merge other PR first then I can do changes after rebase and give you clean results ? or I can add this codes to over there with your changes and also keep clean ?
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.
By other PR
you mean #170?
from supervision.draw.color import Color, ColorPalette | ||
|
||
|
||
class BaseAnnotator: |
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.
I like the BaseAnnotator
idea, but we should drop all text-related properties from the class definition. Not every annotator will draw text.
I'm not even sure we need color
and thickness
is the right choice. I was thinking about making BlurAnnotator
. As the name suggests, it is responsible for blurring detections. If you think about it, it doesn't need color
nor thickness
.
No, please go one by one. One of the reasons I moved so slowly last time was the size of PR (and the amount of other things I need to do at Roboflow). Let's keep the big picture in mind (ideas from the old PR), but move step by step. 🙏🏻 |
I was going to seperate PR already just said "as a next step" so all good. |
@@ -147,6 +133,128 @@ def annotate( | |||
return scene | |||
|
|||
|
|||
class CircleAnnotator(BaseAnnotator): |
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.
By other PR
you mean #170?
Yes I meant #170 |
superseded pull request by #386 |
Description
We have box annotator and for support different shape I added circle annotator and split init section and create base annotator for reduce duplicate code.
List any dependencies that are required for this change.
Type of change
Please delete options that are not relevant.
How has this change been tested, please provide a testcase or example of how you tested the change?