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 mapped int negative or overflow bugs, improve color C API #2349

Merged
merged 4 commits into from
Aug 21, 2023

Conversation

ankith26
Copy link
Member

@ankith26 ankith26 commented Jul 26, 2023

The goal of this PR is to fix #2336 and fix #1408 (the original)

The core issues are that a color is actually Uint32, but long on windows is actually Sint32. This leads to

  • Functions returning a mapped int can return a negative value (values above and including 0x80000000 are casted incorrectly when they are Sint32)
  • Functions expecting a mapped int can sometimes handle this negative value
  • Functions expecting a mapped int can sometimes fail to handle a proper mapped int above 0x80000000

The route I'm taking in this PR that involves least user-facing changes and breakages is

  • No changes to functions returning mapped ints (maybe this could/should be changed, but I'd like to defer that to a future PR)
  • All functions handling mapped int passes through the same internal C API, and this should handle both negative and positive-but-bigger-than-0x80000000 values, and also cap further overflows and error correctly

In the process, I figured color handling API was in a bit of a mess. We don't handle str and int colors in a lot of places. But in this PR I did not want to change anything that can have a user-facing impact (apart from the bug). This bug is related to issues in the python 2 to 3 port.

I introduced unified API for color handling (in a way that doesn't change anything userfacing)

Here is a list of old API, with new API equivalents

  • pg_RGBAFromColorObj(...) (now removed) -> pg_RGBAFromObjEx(..., PG_COLOR_HANDLE_SIMPLE)
  • pg_RGBAFromObj(...) -> pg_RGBAFromObjEx(..., PG_COLOR_HANDLE_SIMPLE)
  • pg_RGBAFromFuzzyColorObj(...) (now removed) -> pg_RGBAFromObjEx(..., PG_COLOR_HANDLE_ALL)

A new addition pg_MappedColorFromObj replaces copypasted block of code that handled int incorrectly (this is what fixes this bug)

The final goal is to get rid of these temporary flags entirely, as every function should handle all ColorValues. But that involves writing more docs and tests, so it's coming in a future PR.

@ankith26 ankith26 requested a review from a team as a code owner July 26, 2023 07:41
@ankith26 ankith26 added Code quality/robustness Code quality and resilience to changes color pygame.color bugfix PR that fixes bug labels Jul 26, 2023
@ankith26 ankith26 changed the title Fix mapped int negative bugs, improve color C API Fix mapped int negative or overflow bugs, improve color C API Jul 26, 2023
Copy link
Member

@zoldalma999 zoldalma999 left a comment

Choose a reason for hiding this comment

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

Overall, I like this pr, makes handling color-like objects easier. Left a few questions.

Also, when we planning to change PG_COLOR_HANDLE_SIMPLE calls to handle everything?

src_c/pixelarray.c Show resolved Hide resolved
docs/reST/c_api/base.rst Show resolved Hide resolved
src_c/_pygame.h Outdated Show resolved Hide resolved
src_c/color.c Outdated Show resolved Hide resolved
@ankith26 ankith26 added this to the 2.3.2 milestone Aug 10, 2023
@MyreMylar
Copy link
Member

For whatever reason CircleCI here is trying to build with python 3.6 - which we don't support - so it is failing.

Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

The new API makes sense, and the color tests run locally for me. I also couldn't find any lingering usages of the old ways.

Copy link
Contributor

@dr0id dr0id left a comment

Choose a reason for hiding this comment

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

removing code duplication is always good and makes handling colors more consistent.

LGTM

@ankith26 ankith26 merged commit 83b4399 into main Aug 21, 2023
32 checks passed
@ankith26 ankith26 deleted the ankith26-color-api branch August 21, 2023 14:12
ankith26 added a commit that referenced this pull request Sep 2, 2023
Fix mapped int negative or overflow bugs, improve color C API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug Code quality/robustness Code quality and resilience to changes color pygame.color
Projects
None yet
5 participants