-
-
Notifications
You must be signed in to change notification settings - Fork 168
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
Fix round rect with negative values #2303
base: main
Are you sure you want to change the base?
Conversation
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.
Hello, thanks for the PR.
As I'm not experienced in these parts of the docs, I'm stuggling to understand even the smallest diffs 😅
I left some questions as reviews
@@ -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); |
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.
Instead of simply ignoring negative values here, we could raise an error if this is negative instead
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.
That could be another solution indeed
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.
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.
|
||
if (width > rect->w / 2 || width > rect->h / 2) { | ||
width = MAX(rect->w / 2, rect->h / 2); | ||
width = 0; |
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'm not sure how this change is helping fix the issue, it looks like it will change behaviour against what is documented
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.
It doesn't do much. I do it just because of this line
Line 2497 in 2e864cd
if (width == 0) { /* Filled rect */ |
Line 2497 in 2e864cd
if (width == 0) { /* Filled rect */ |
Line 2525 in 2e864cd
else { |
bottom_left = radius; | ||
if (bottom_right < 0) | ||
if (!bottom_right) |
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.
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
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.
Wait a second, you are probably right. Gonna check tomorrow.
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.
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
.
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.
Looks like this needs more work to finish off.
Fix #2285.
During this I discovered drawing round rect and circle quadrant have another issue not related to this one, but that would require another PR.