-
-
Notifications
You must be signed in to change notification settings - Fork 162
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
transform.solid_overlay #2304
transform.solid_overlay #2304
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
That isn't really this PRs fault, though it's something I plan to tackle in the near future |
As per @ScriptLineStudios on Discord I'll take over this PR. |
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.
OK, LGTM.
All works locally for me, I like the keep_alpha
flag and I can think of lots of uses for this in games. Also everything is passing CI now and the docs make sense.
I'm not sure there is much scope for SIMD in this transform either as it is a per pixel branch rather than any mathematical operation on every pixel.
Happy to see this go in.
correct docs and update versionadded
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 think the message I sent on discord got lost, so I'm typing it here instead.
Basically providing a string color such as "green"
raises a TypeError: invalid search_color argument
, which is not expected as the argument supports ColorValue
, and the error is also referring to a non existent parameter search_color
On a side note, I'd like for this function to have some kind of alpha threshold parameter, but it's fine if it doesn't (the first time I tested my image had an alpha of 6 which I wasn't aware of, and I had to open an image editing program to bring it to 0 before trying again.)
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.
the search_color in the error is still present but this is such a small nitpick that I'm approving anyways. I'm gonna let some SR or SC member merge it as this is new api. LGTM, and works locally.
@@ -20,6 +20,7 @@ def rotate(surface: Surface, angle: float) -> Surface: ... | |||
def rotozoom(surface: Surface, angle: float, scale: float) -> Surface: ... | |||
def scale2x(surface: Surface, dest_surface: Optional[Surface] = None) -> Surface: ... | |||
def grayscale(surface: Surface, dest_surface: Optional[Surface] = None) -> Surface: ... | |||
def solid_overlay(surface: Surface, color: ColorValue, dest_surface: Optional[Surface] = None, keep_alpha: bool = False) -> Surface: ... |
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.
This line is too long, maybe you could re-format this file with black/ruff?
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.
This should be fixed now thanks.
src_c/transform.c
Outdated
} | ||
} | ||
|
||
SDL_UnlockSurface(newsurf); |
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.
Why is it an unmatched lock here?
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.
This is the original sin lol, always been there so it became invisible to my eyes, should be fixed now.
Locking/Unlocking should now be more correct. Previously we supported any Bpp surfaces in the general path but realistically only 16 bit surfaces are left after the filtering we do before, hence the simplifications.
@itzpr3d4t0r after typing.py, ColorValue isn't available anymore. If you can you can update the branch and replace ColorValue with ColorLike from pygame.typing |
I'm not sure i can do it right atm. I've always committed from github because of commit permissions being broken in my ide for some reason... so I'm afraid someone else has to do it. |
closes #1847
produces: