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

LineZoneAnnotator: Align text to line counter in non-horizontal lines #854

Merged
merged 25 commits into from
Sep 26, 2024

Conversation

jcruz-ferreyra
Copy link
Contributor

Description

In this pull request, I've enhanced LineZoneAnnotator class in supervision/detection/line_counter.py to align text with the LineZone object even when it is non-horizontal.

Additionally, I've introduced the option to omit drawing the text box behind the text, leveraging the optional condition on the background_color attribute in the draw_text method in supervision/draw/utils.py. Furthermore, users can now choose whether to draw the count centered in the LineZone object or aligned with its end, offering a solution to prevent overlaps in case of multiple LineZone objects in the same frame.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How has this change been tested, please provide a testcase or example of how you tested the change?

You can explore the results of the changes made in this Colab notebook. I've tested LineZone objects with varying orientations and positions within the frame, as well as LineZoneAnnotator objects with different values for draw_text_box and draw_centered parameters.

NOTE: Personally, I don't fully like the circles drawn at the extremes of the LineZone objects. However, I chose not to modify them to maintain conciseness in the pull request. Given the introduction of the new is_point_in_limits method in the LineZone class, it might be more effective to draw a small line perpendicular to the main line (or even nothing at all) to emphasize this new functionality.

Any specific deployment considerations

No specific deployment considerations. Just one parameter added as an attribute for LineZone Class with default value "both" mantaining previous implementation.

Docs

No changes made to Docs

@jcruz-ferreyra jcruz-ferreyra changed the title LineZoneAnnotator: Align text to line coutner in non-horizontal lines LineZoneAnnotator: Align text to line counter in non-horizontal lines Feb 5, 2024
@SkalskiP
Copy link
Collaborator

SkalskiP commented Feb 7, 2024

Hi @jcruz-ferreyra 👋🏻 Thanks a lot of opening the new PR.

@jcruz-ferreyra
Copy link
Contributor Author

Hi @SkalskiP! I was wondering if you've been able to check the PR

@LinasKo
Copy link
Collaborator

LinasKo commented May 31, 2024

Hi @jcruz-ferreyra 👋

Thanks for keeping the branch up to date. I've seen your contribution before. I think it's very high quality and I believe we'll have time to look deeper in about a week.

Thanks again for an awesome contribution :)

@LinasKo
Copy link
Collaborator

LinasKo commented Sep 20, 2024

Hi @jcruz-ferreyra 👋

It's been a while, apologies for that. I'm planning to include this in the next supervision release. I'm happy to take over from this point onwards.

Ahead of that, I moved some things around, cleaned up a few things.
Here's the Colab I used for testing: Colab

There's a few decisions to be made:

1. Is this too complicated?

I cannot deny the visuals are perfect. It looks 10x better than what we had. However, the code is inevitably more cumbersome, and we probably want to move it to utils somewhere, or remove a few of these features.

2. Is this too slow?

On a Colab, we can draw the new line zone 60 times a second, whereas previously we could do it 2000 times. At this point, realtime performance would be impacted, which I'd like to minimize.

3. Do we need the feature to draw off-center?

It adds much code, and it wasn't widely requested so far.

4. Default code to non-centered.

We should definitely default to the prior method when orientation is not required. It's more efficient.

I'll park this for a few days and come back to finish it up later on.

@jcruz-ferreyra
Copy link
Contributor Author

Hi @LinasKo ,

I'm really glad you had the chance to review the pull request and found it interesting! I completely understand the concern regarding performance—it's unfortunate that OpenCV doesn't natively support drawing diagonal text. However, I still believe this functionality is crucial for non-standard use cases where lines need to be drawn in multiple directions.

I also appreciate the improvements in code quality introduced in the latest commit. Please don't hesitate to let me know if there's anything I can do to assist you further with this. I'd be happy to contribute to any little thing I can, especially after all the help that Supervision has provided me with my CV tasks.

@LinasKo
Copy link
Collaborator

LinasKo commented Sep 25, 2024

Hey @jcruz-ferreyra 👋

I'm just putting a few finishing touches on the PR. I found ways to make the performance sane and the code more compact - we're keeping it, merging it, and retaining all the features you contributed 😉

Indeed, shame about OpenCV's lack of diagonality here. I've looked around, and everyone suggests a similar solution to what you added.

Thank you for volunteering to help out!
Feel free to send a PR our way if you spot a way to make the library better. Also, I haven't decided yet, but we may post a few issues for Hacktoberfest soon.

As for this PR, there's not much work left. I'll take over and get it done. Thank you for the contribution! 🚀

@LinasKo
Copy link
Collaborator

LinasKo commented Sep 25, 2024

Leaving this for tomorrow. It feels like there's something slightly wrong with _draw_basic_label - the normal, unrotated labels are too close together.

Docs are checked. Same Colab used for testing.

Left to check: does commenting out the following code changes the distance between labels? Ideally it shouldn't.

# Make sure text is displayed upright
if line_zone.vector.start.x > line_zone.vector.end.x:
    label_image = cv2.flip(label_image, flipCode=-1)

@LinasKo
Copy link
Collaborator

LinasKo commented Sep 26, 2024

I've checked the docs, logic, code complexity and performance. Merging!

Thank you for a high quality contribution, @jcruz-ferreyra!
Drop a link to your LinkedIn if you wish to be mentioned in the next release 😉

@LinasKo LinasKo merged commit e63611d into roboflow:develop Sep 26, 2024
9 checks passed
@jcruz-ferreyra
Copy link
Contributor Author

Hey @LinasKo, those are great news! I'm glad the contribution was well received. Here's my LinkedIn profile:
https://www.linkedin.com/in/jcruz-ferreyra/

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.

3 participants