Border radius for pygame.draw.polygon (resolves #1527) #2610

ezzakri-anas · 2023-12-17T21:12:05Z

This pull request resolves the enhancement issue #1527 (previously #3011) about implementing the border radius for pygame.draw.polygon

Overview

Added a new parameter border_radius to the pygame.draw.polygon function to specify the border radius of the rounded polygon corners.
Updated the list of keywords in pygame.draw.polygon to include "border_radius" in the function signature.
Added a new function draw_round_polygon to handle the drawing of rounded polygons. This function takes care of the logic for calculating rounded corners.
Integrated this function as a part of polygon in draw.c.

Geometry Calculations:

Implemented various geometric calculation functions, such as find_parallel_line, project_point_onto_segment, and intersection(to calculate intersections between two lines), angle, side ….

draw_round_polygon Function:

**static void draw_round_polygon(SDL_Surface *surf, int *pts_x, int pts_y, int radius, int num_points, Uint32 color, int drawn_area)

Purpose:

This function is responsible for drawing a rounded polygon with a specified border radius. It calculates and draws circular segments at each corner of the polygon.
Parameters:

surf: The target surface to draw on.
pts_x: Array of x-coordinates of polygon vertices.
pts_y: Array of y-coordinates of polygon vertices.
border_radius: The border radius of the rounded corners.
num_points: Number of vertices in the polygon.
color: Color of the polygon.
drawn_area: Array to store the bounding box of the drawn area.

Details:

Path calculation loop:

Coordinate Initialization: Initializes arrays and structures to store calculated points and circles.
Parallel Line Calculation: The find_parallel_line function calculates two parallel lines given three points: the current vertex and its adjacent vertices. The function ensures that the parallel lines are on the correct side of the vertex.
Circle and Path Calculation: The center of the circular segment is found by calculating the intersection of two parallel lines (line1 and line2) using the intersection function.
The circular arc's starting and ending points are obtained by projecting the center onto adjacent line segments using the project_point_onto_segment function.
Stores these points in the path array.

Drawing loop:

For each segment in the path array, it calculates start and end angles and draws the circular arc using the draw_arc function.
Draws a straight line between consecutive circular arcs.

Debugging Output:

During the calculation loop, if three consecutive points are aligned the loop is interrupted with a return statement to abort the function.
A condition on the border_radius is verified on each vertex, at each iteration, to avoid incorrect drawing. if the condition is not satisfied the function is aborted.
As the draw_round_polygon function returns void (a choice made to align with the same logic of other drawing functions), it is not possible to raise an error directly from draw_round_polygon. Instead, we settle for a return statement whenever an error should be raised to abort the function, and a printf of the error message.

oddbookworm

I see lots of undocumented math that isn't immediately obvious. Please make sure you're leaving enough comments that someone else can come in later and make fixes/changes without having to reinvent your entire thought process.

I also don't see any tests written. You should write tests to make sure that your code is behaving as expected. They live in the test directory.

Please update documentation (as I outline in one of my comments)

Formatting of the code can be wacky in a few places, please run python setup.py format

No stub updates. Since you're changing the signature of a function, you should update the appropriate stub in buildconfig/stubs/pygame

src_c/doc/draw_doc.h

src_c/draw.c

oddbookworm · 2023-12-17T21:19:59Z

src_c/draw.c

@@ -721,19 +725,20 @@ polygon(PyObject *self, PyObject *arg, PyObject *kwargs)
    Uint32 color;
    int *xlist = NULL, *ylist = NULL;
    int width = 0; /* Default width. */
+    int border_radius = 0; /* Default border_radius. */


This isn't a super important nitpick, but is there a reason this should be a signed integer vs unsigned? What do you expect to happen if a negative border_radius is fed in?

I unresolved this because you didn't actually respond to it. If you're going to resolve a comment, there better be some kind of followup. If it doesn't warrant a change, then add a comment to reply. Since I was asking a question, I would expect an answer. Marking this as resolved without doing that signifies to me that you've done something to address it. In this case, you have done nothing, and my questions are still valid.

I apologize for any confusion. I provided an explanation in the draw.rst file regarding the behavior of the function when border_radius is negative. I assumed this information would be visible and that it would be enough to resolve the comment. I'll make sure to be clearer in my responses.
Regarding the border_radius parameter, similar to the approach with the width parameter, negative values are not considered, and the function does not apply any radius while still drawing the polygon. The border_radius parameter becomes active only when it is positive.

src_c/draw.c

Matiiss · 2023-12-18T22:16:44Z

If you need to raise exceptions from your helper function, I would suggest making it return -1 on failure and 0 on success (as Python C API functions do for the most part).
Before returning -1, you can call PyErr_SetString with the appropriate exception type (seemingly PyExc_ValueError) and the message and then return -1;
Then in the "main" polygon function, check for == -1 and return NULL;

itzpr3d4t0r · 2023-12-19T11:33:31Z

You have to add a bunch of explicit casts:

oddbookworm

Still got quite a few things I'd like to see changed. You also still have a bunch of implicit conversion warnings which get treated as errors in some of our CI.

    src_c/draw.c(3110): error C2220: the following warning is treated as an error
    src_c/draw.c(3110): warning C4244: 'initializing': conversion from 'double' to 'int', possible loss of data
    src_c/draw.c(3111): warning C4244: 'initializing': conversion from 'double' to 'int', possible loss of data
    src_c/draw.c(3112): warning C4244: 'initializing': conversion from 'double' to 'int', possible loss of data
    src_c/draw.c(3113): warning C4244: 'initializing': conversion from 'double' to 'int', possible loss of data
    src_c/draw.c(3114): warning C4244: 'initializing': conversion from 'double' to 'int', possible loss of data
    src_c/draw.c(3115): warning C4244: 'initializing': conversion from 'double' to 'int', possible loss of data

oddbookworm · 2023-12-20T03:13:27Z

src_c/draw.c

@@ -721,19 +725,20 @@ polygon(PyObject *self, PyObject *arg, PyObject *kwargs)
    Uint32 color;
    int *xlist = NULL, *ylist = NULL;
    int width = 0; /* Default width. */
+    int border_radius = 0; /* Default border_radius. */


I unresolved this because you didn't actually respond to it. If you're going to resolve a comment, there better be some kind of followup. If it doesn't warrant a change, then add a comment to reply. Since I was asking a question, I would expect an answer. Marking this as resolved without doing that signifies to me that you've done something to address it. In this case, you have done nothing, and my questions are still valid.

src_c/draw.c

test/draw_test.py

…rror handling and return types.

ezzakri-anas · 2023-12-24T20:04:18Z

I've implemented the test cases for rounded polygons within separate functions inside the DrawPolygonMixin class. I'm not sure if this aligns with your preferences or if you would prefer them in a different class. Also,polygon.draw.arc function lacks tests to ensure accurate arc rendering. Given this, I'm unsure if testing the rounded polygons for correct rounding is coherent, as the function uses polygon.draw.arc for drawing the roundness. Any suggestions or guidance on this matter would be appreciated.

Matiiss · 2023-12-25T14:55:55Z

I've implemented the test cases for rounded polygons within separate functions inside the DrawPolygonMixin class. I'm not sure if this aligns with your preferences or if you would prefer them in a different class. Also,polygon.draw.arc function lacks tests to ensure accurate arc rendering. Given this, I'm unsure if testing the rounded polygons for correct rounding is coherent, as the function uses polygon.draw.arc for drawing the roundness. Any suggestions or guidance on this matter would be appreciated.

I think it's fine if they are in that class. As for arc tests, that will have to be added in a different PR. You can still check for rounded corners with your polygon though, because it also does a bit of implicit arc testing which just adds extra tests as a safety measure pretty much. Testing is one of those places where redundancy is fine.

ezzakri-anas · 2023-12-25T15:02:22Z

I've implemented the test cases for rounded polygons within separate functions inside the DrawPolygonMixin class. I'm not sure if this aligns with your preferences or if you would prefer them in a different class. Also,polygon.draw.arc function lacks tests to ensure accurate arc rendering. Given this, I'm unsure if testing the rounded polygons for correct rounding is coherent, as the function uses polygon.draw.arc for drawing the roundness. Any suggestions or guidance on this matter would be appreciated.

I think it's fine if they are in that class. As for arc tests, that will have to be added in a different PR. You can still check for rounded corners with your polygon though, because it also does a bit of implicit arc testing which just adds extra tests as a safety measure pretty much. Testing is one of those places where redundancy is fine.

In this context, is it necessary to test for the precise radius of roundness, or would it suffice to confirm that the corners are indeed rounded?

ezzakri-anas · 2023-12-27T17:40:00Z

I don't get why the circleci failed while all i did is just add some tests.

ankith26 · 2024-01-02T17:10:09Z

That is not your PRs fault, don't worry. It was a separate issue that has been fixed now

MyreMylar

It looks like the rounded corners are being calculated in the wrong place:

The outside edge of the corner is lining up with the middle of the sides rather than the outside edge of them and that is throwing it off. I'm guessing there is a wrong parameter somewhere. Otherwise things look alright.

I'm going to put this on 'request changes'. If the original author doesn't want to work on it anymore (this has been stuck since December I think) then somebody else feel free to take it over and fix that bug.

MyreMylar · 2024-05-25T20:07:47Z

test/draw_test.py

+        surface = pygame.Surface((surfw, surfh))
+        surface.fill(surface_color)
+
+        self.draw_polygon(surface, polygon_color, LARGE_SQUARE, 1, 5)


I'm guessing we need some tests like this with a width other than "1"

bilhox · 2024-09-28T12:12:57Z

Hi @ezzakri-anas , do you want to revive this PR ?

Dahbani1 and others added 2 commits December 17, 2023 21:56

Added geometrical calculations for draw_round_polygon

6e8840c

Added draw_round_polygon and changed DOC_DRAW_POLYGON

f710d03

ezzakri-anas requested a review from a team as a code owner December 17, 2023 21:12

oddbookworm requested changes Dec 17, 2023

View reviewed changes

Starbuck5 added New API This pull request may need extra debate as it adds a new class or function to pygame draw pygame.draw labels Dec 17, 2023

Comments added

ed7c02b

Dahbani1 and others added 4 commits December 19, 2023 00:23

Comments added

16e6ac5

Fixed error handling, Added dynamic array handling with malloc

646ce2f

Updated documentation

39fae05

Updated stub

61f5b99

ezzakri-anas requested a review from oddbookworm December 19, 2023 02:00

ezzakri-anas added 5 commits December 19, 2023 13:58

Resolved type conversion

0e489dd

Resolved integer conversion

ac168b1

Removed unnecessary include for test

cc23636

Added tests

faf9922

Fixed format

d82da31

ezzakri-anas requested a review from itzpr3d4t0r December 19, 2023 21:57

oddbookworm requested changes Dec 20, 2023

View reviewed changes

ezzakri-anas added 2 commits December 20, 2023 16:25

Added comments, changed function name and modified condition

7e7a525

Refactored intersection and find_parallel_line functions to improve e…

f2bf113

…rror handling and return types.

ezzakri-anas requested a review from oddbookworm December 23, 2023 19:43

Added tests

47ae925

Fixed format

277a00d

ezzakri-anas requested a review from Matiiss December 24, 2023 20:51

Added test for drawing rounded square, diamond, rhombus and cross

65dd610

MyreMylar closed this May 12, 2024

MyreMylar reopened this May 12, 2024

MyreMylar closed this May 23, 2024

MyreMylar reopened this May 23, 2024

Merge branch 'main' into round_polygon

5b160ee

MyreMylar requested a review from a team as a code owner May 25, 2024 19:45

MyreMylar requested changes May 25, 2024

View reviewed changes

MyreMylar reviewed May 25, 2024

View reviewed changes

Uh oh!

Border radius for pygame.draw.polygon (resolves #1527) #2610

Are you sure you want to change the base?

Border radius for pygame.draw.polygon (resolves #1527) #2610

Conversation

ezzakri-anas commented Dec 17, 2023

This pull request resolves the enhancement issue #1527 (previously #3011) about implementing the border radius for pygame.draw.polygon

Overview

Geometry Calculations:

draw_round_polygon Function:

Purpose:

Details:

Path calculation loop:

Drawing loop:

Debugging Output:

Uh oh!

oddbookworm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oddbookworm Dec 17, 2023

Choose a reason for hiding this comment

Uh oh!

oddbookworm Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

ezzakri-anas Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Matiiss commented Dec 18, 2023

Uh oh!

itzpr3d4t0r commented Dec 19, 2023

Uh oh!

oddbookworm left a comment

Choose a reason for hiding this comment

Uh oh!

oddbookworm Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ezzakri-anas commented Dec 24, 2023

Uh oh!

Matiiss commented Dec 25, 2023

Uh oh!

ezzakri-anas commented Dec 25, 2023

Uh oh!

ezzakri-anas commented Dec 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

I don't get why the circleci failed while all i did is just add some tests.

Uh oh!

ankith26 commented Jan 2, 2024

Uh oh!

MyreMylar left a comment

Choose a reason for hiding this comment

Uh oh!

MyreMylar May 25, 2024

Choose a reason for hiding this comment

Uh oh!

bilhox commented Sep 28, 2024

Uh oh!

Uh oh!

ezzakri-anas commented Dec 27, 2023 •

edited

Loading