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

Add polygon rotation functions #1098

Closed

Conversation

sharingan000
Copy link
Contributor

Description

I wrote two new functions and one auxiliary one, all of them are designed to rotate polygons at a certain angle, around a certain point. in the '2d' function you can rotate a polygon in a plane around a given point or around the center of the polygon. And the '3d' function performs rotation in three-dimensional space along any axis and for this reason will return 3-dimensional coordinates

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?

google disk link - https://drive.google.com/file/d/1CrThP7A8cXzBbULWdneV5Eb3AOkvtq2l/view?usp=sharing
I recommend testing in Jupyter notebook

@LinasKo
Copy link
Collaborator

LinasKo commented Apr 8, 2024

Hi @sharingan000 👋

Unfortunately, I'm tempted to close this one. The possible use cases for the end-users of Roboflow, the set of problems they can solve with this algo that would be related to what supervision provides (2D object detection, segmentation, annotation), isn't clear.

@SkalskiP, what's your take on this one?

@SkalskiP
Copy link
Collaborator

SkalskiP commented Apr 8, 2024

Hi @sharingan000 and @LinasKo 👋🏻 I must admit, I also can't think of any use case when we would need rotating polygons, and for this reason, I'm afraid we will have to close this PR. The only thing that comes to my mind is dataset augmentation, but at the moment, I do not plan to open such a front of work.

@sharingan000 I see your interest in mathematical/geometrical issues. Our InferenceSlicer currently utilizes non_max_suppression to filter out redundant detections, which primarily appear on patch overlap. We are looking for solutions that, instead of removing duplicates, would merge them. Would you be interested in assisting us in this area?

@SkalskiP SkalskiP closed this Apr 8, 2024
@sharingan000
Copy link
Contributor Author

Hi @sharingan000 and @LinasKo 👋🏻 I must admit, I also can't think of any use case when we would need rotating polygons, and for this reason, I'm afraid we will have to close this PR. The only thing that comes to my mind is dataset augmentation, but at the moment, I do not plan to open such a front of work.

@sharingan000 I see your interest in mathematical/geometrical issues. Our InferenceSlicer currently utilizes non_max_suppression to filter out redundant detections, which primarily appear on patch overlap. We are looking for solutions that, instead of removing duplicates, would merge them. Would you be interested in assisting us in this area?

I would really like to help you with this, I just want to understand the problem a little better

@sharingan000 sharingan000 deleted the geometry-utils-new-features branch April 8, 2024 23:02
@SkalskiP
Copy link
Collaborator

SkalskiP commented Apr 9, 2024

That's awesome, @sharingan000! 🔥

The InferenceSlicer consists of two key elements: a moving window that slices the image into smaller, partially overlapping patches and a postprocessing algorithm that primarily addresses duplicate detections occurring at the edges of patches. You can see the InferenceSlicer in action in this how-to guide.

Currently, we use non-max suppression to filter out double detections, but there are other, often better, methods for handling these extra detections. NMS removes overlapping detections, but there are methods that involve merging them instead. In the past, members of the supervision community have proposed two alternative solutions:

Would you be interested in implementing one of these methods? Initially, I would like us to implement weighted_box_fusion or box_non_max_merging as a standalone utility, and in the next PR, integrate it into the logic of InferenceSlicer. Does this sound interesting?

@LinasKo
Copy link
Collaborator

LinasKo commented Apr 9, 2024

Small update @SkalskiP and @sharingan000,

The contributor for #500 PR is keen to continue, so it's only the weighted fusion that we'd like to see.

Let's move further discussions to #268 - I'll post an explanation there shortly.

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