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

display.set_gamma_ramp() unit test #2039

Merged
merged 2 commits into from Sep 5, 2020

Conversation

anguye13
Copy link
Contributor

@MightyJosip MightyJosip linked an issue Aug 11, 2020 that may be closed by this pull request
@MyreMylar
Copy link
Contributor

MyreMylar commented Aug 17, 2020

I feel like this test doesn't provide a great example of what setting the gamma ramp actually does. My intuition (based on no knowledge or testing) is that you set a look up table for each channel (e.g. red[50] now equals 60 instead of 50) and then that is applied to the display surface somehow. I assume it happens as some sort of final step when you .flip()/.update() the screen but having that show in a test somehow would be useful even if it is in a separate test that will only run locally. If it is a post .update()/flip() change to the display, that is tricky to test in the standard ways, like examining the pixels of the surface (as they will still be the same).

So, failing any other good ideas coming to mind, my thought is that you could keep the current test but add a local interactive test that has a couple of dramatic gamma shifts displayed (e.g. start with a black screen shift blue[0] to 255) and ask users to confirm via the console that the window contents are blue/red/green or whatever ramp is set. Does that make sense?

You can see examples of interactive tests in the cdrom or joystick test modules. They are basically a special type of test that only runs when the tests are run locally on a users machine and prompt for feedback.

Copy link
Contributor

@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.

As above.

@anguye13
Copy link
Contributor Author

Yea that makes sense, I’ll try and make the changes when I get a chance 👍

@illume
Copy link
Member

illume commented Sep 5, 2020

I think this test provides value, and that we can merge it in. I've added the extra test requirements into the #1751 issue (which we can leave open).

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

Thanks :)

@illume illume merged commit 5718060 into pygame:master Sep 5, 2020
@illume illume changed the title Added unit test for display.set_gamma_ramp() display.set_gamma_ramp() unit test Sep 15, 2020
@illume illume added the display pygame.display label Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display pygame.display
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants