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

Bug Fix for the perfect Score #36

Merged
merged 1 commit into from
Feb 13, 2023
Merged

Bug Fix for the perfect Score #36

merged 1 commit into from
Feb 13, 2023

Conversation

jriyyya
Copy link
Contributor

@jriyyya jriyyya commented Feb 12, 2023

Overview

Fixed a bug : Perfect score was shown incorrectly due to incomplete colortest

Steps to replicate bugs

  1. Play the game and make it so that the stick lands on a white pixel
  2. Notice that the game shows +1 perfect even though we didn't make the stick land at the red spot in the center of a pillar

Expected Behavior

The game is only supposed to show perfect when we land on a red pixel

Cause

The colortest for perfect score was done in the following manner

if(colortest[0] == 255):

which only checks the Red value in the RGB spectrum
so even a whitespace would match the check as Red value of white is also 255 as is for any other color with R value of 255

Fix

To fix this I have simply set the colortest to check all r g and b values of the rgb spectrum in the following manner

if(colortest[0] == 255 and colortest[1] == 0 and colortest[2] == 0):

this ensures red and only red pixels trigger the perfect score

Screenshot

screenshot1

Here is a demonstration of how the bug plays out

@quozl quozl merged commit f946570 into sugarlabs:master Feb 13, 2023
@quozl
Copy link
Contributor

quozl commented Feb 13, 2023

Thanks. It's the sort of thing that would be best done as a new method or function, so while your solution is not perfect it is better than before. With the change merged it isn't very obvious from the code what the test is for.

@jriyyya
Copy link
Contributor Author

jriyyya commented Feb 15, 2023

will try to refactor this and look into it in the manner you specified

@jriyyya
Copy link
Contributor Author

jriyyya commented Feb 18, 2023

Hi @quozl I was wondering as to what you mean exactly, when you say it'd be better to turn this into a function or a methods as judging by the rest of the code it seems there are no modular function definitions and there's only game class in the entire main.py

@quozl
Copy link
Contributor

quozl commented Feb 18, 2023

Nothing exactly. My point is that the code you added is not easily explained.

Looking at the rest of the file main.py, there's two calls to get_at which return an RGBA tuple
or colour (depending on Pygame version). There's three tests of the value returned. Each test compares tuple members.

Perhaps it would be better to define a Color or tuple in game.make and compare against it. There is already a white and black. The code would be more easily explained that way.

But still, let's not make the perfect the enemy of the good.

Had you used our commit message convention, I probably would have read your commit message to understand the change. See https://github.com/sugarlabs/sugar-docs/blob/master/src/contributing.md#making-commits

@jriyyya
Copy link
Contributor Author

jriyyya commented Feb 19, 2023

Perhaps it would be better to define a Color or tuple in game.make and compare against it. There is already a white and black. The code would be more easily explained that way.

Initially I was thinking about the same.

But still, let's not make the perfect the enemy of the good.

Quite understandable

Had you used our commit message convention, I probably would have read your commit message to understand the change. See https://github.com/sugarlabs/sugar-docs/blob/master/src/contributing.md#making-commits

Thanks, I went through this, Next time, will make better commit message :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants