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.

Sign up for GitHub

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

Fix round rect with negative values #2303

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fix round rect with negative values #2303

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22a9a7d
Fix round rect with negative values
MightyJosip Jul 11, 2023
af8b7bd
Fix round rect with negative values
MightyJosip Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src_c/draw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,9 +972,15 @@ rect(PyObject *self, PyObject *args, PyObject *kwargs)
rect->y += rect->h;
rect->h = -rect->h;
}
// Ignore negative values
radius = MAX(radius, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of simply ignoring negative values here, we could raise an error if this is negative instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be another solution indeed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

silently clamping it to 0 vs raising an error

I'm not sure which is preferable when using the code. I tend to be against silent things. Make is explicit, not implicit.

top_left_radius = MAX(top_left_radius, 0);
top_right_radius = MAX(top_right_radius, 0);
bottom_left_radius = MAX(bottom_left_radius, 0);
bottom_right_radius = MAX(bottom_right_radius, 0);

if (width > rect->w / 2 || width > rect->h / 2) {
width = MAX(rect->w / 2, rect->h / 2);
width = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how this change is helping fix the issue, it looks like it will change behaviour against what is documented

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't do much. I do it just because of this line

pygame-ce/src_c/draw.c

Line 2497 in 2e864cd

if (width == 0) { /* Filled rect */
Basically if width == 0, draw filled rect. With old width=MAX(...) you would still get filled rect, but it wouldn't go in

pygame-ce/src_c/draw.c

Line 2497 in 2e864cd

if (width == 0) { /* Filled rect */
but rather in

pygame-ce/src_c/draw.c

Line 2525 in 2e864cd

else {

}

draw_round_rect(surf, rect->x, rect->y, rect->x + rect->w - 1,
Expand Down Expand Up @@ -2472,13 +2478,13 @@ draw_round_rect(SDL_Surface *surf, int x1, int y1, int x2, int y2, int radius,
{
int pts[16], i;
float q_top, q_left, q_bottom, q_right, f;
if (top_left < 0)
if (!top_left)
top_left = radius;
if (top_right < 0)
if (!top_right)
top_right = radius;
if (bottom_left < 0)
if (!bottom_left)
bottom_left = radius;
if (bottom_right < 0)
if (!bottom_right)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand from the docs

if any of these values are 0, the corner should have no rounded, and if the value is negative (default) it should "inherit" radius. This fix seems to be breaking whatever is documented in my understand, correct me if I'm missing something

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait a second, you are probably right. Gonna check tomorrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He is right. This way a border_top_right_radius radius of 0 would inherit the value of border_radius whereas before it would set that corner to have no rounding.

I could imagine this breaking usage if somebody used the popular GUI button pattern of three rounded corners (set via border_radius and one at right angles set with border_top_right_radius.

bottom_right = radius;
if ((top_left + top_right) > (x2 - x1 + 1) ||
(bottom_left + bottom_right) > (x2 - x1 + 1) ||
Expand Down