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

Fix handling all color types in pygame.transform.threshold #3156

Merged

Conversation

damusss
Copy link
Contributor

@damusss damusss commented Oct 8, 2024

The c internal function _color_from_obj only allows mapped colors for some reason, but all color arguments in the stubs for pygame.transform.threshold pretend to allow ColorLike which also allows strings. My modification makes sure all the promised color types are allowed and add a test to make sure it doesn't break in the future.

pygame.transform.threshold should be the only function using the internal c function because all new api coming for transform is going to use the pygame C api function by default.

@damusss damusss added transform pygame.transform bugfix PR that fixes bug labels Oct 8, 2024
@damusss damusss added this to the 2.5.2 milestone Oct 8, 2024
@damusss damusss requested a review from a team as a code owner October 8, 2024 17:28
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR 🎉

As a minor side note, maybe we could add a line in the docs about this, as it is technically new behaviour.

@damusss
Copy link
Contributor Author

damusss commented Oct 10, 2024

It is new behaviour but it wasn't a common and long awaited bugfix like the one about filling with negative topleft area, the c code was just betraying the stubs (someone encountering a value error before this fix should have reported it). Stub-wise, nothing changed, so I think it's not too big of a deal to add a line in the docs. A note in the changelog for 2.5.2 should suffice the people that were using the function anyways.

Copy link
Member

@Temmie3754 Temmie3754 left a comment

Choose a reason for hiding this comment

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

Looks and runs fine for me. 👍

@damusss damusss merged commit 908d3ef into pygame-community:main Oct 10, 2024
28 checks passed
@damusss damusss deleted the fix-_get_color_from_obj-transform branch October 10, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug transform pygame.transform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants