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

Sound code total remake #74

Merged

Conversation

Mohamed-Waiel-Shikfa
Copy link
Contributor

Remade Sound to natively use pygame instead of using subprocesses for a better more robust way to play multiple sound at the same time with no noticeable effect for user. Of course, the URL argument of sound was fixed at the same time and now expects a normal file path.

Implemented Sound using `pygame.mixer.sound` and `pygame.mixer.channel` instead of `pygame.mixer.music` for a better way to play multiple sound at the same time with no noticeable effect on user normal usage.
Will now find the file specified by url in all cases, be it the url is absolute or relative and user is on windows or mac, as long as it exists and gives a nice error message if file not found.
@Mohamed-Waiel-Shikfa
Copy link
Contributor Author

Removed small code section related to the previous way of handling sound using subprocesses.
Quick fix to make sounds work in repl as long as cmu_graphics.run() or runApp() have been called.
The pygame mixer will now initialise with the creation of the first sound object, making it independent of starting the canvas cmu_graphics and possible to play sounds only.
Now handles sound from online sources by checking if url starts with http.
Enhanced robustness by adding backwards compatibility for file paths (including file:// defined paths) and implementing multiple retries for web requests to mitigate network errors.
@Mohamed-Waiel-Shikfa
Copy link
Contributor Author

Mohamed-Waiel-Shikfa commented Jun 10, 2024

Hello!
So, tests are failing but I have done some reading and the issue might be because there is no audio output device connected to the machine running the test.
The code works fine on my laptop so I am not sure what we can do about the tests. Please let me know so that we can merge these changes afterwards :-)

(Here is what I read: https://stackoverflow.com/questions/62125225/pygame-error-mixer-not-initialized-how-to-fix)

@Yuxiang-Huang
Copy link
Contributor

The currently released version of desktop CMU graphics supports multiple sounds, but this functionality is no longer available after this remake. For example with the code below, if I press the mouse, the background music will stop in this branch, but the background music continues in the main branch.

from cmu_graphics import *

background_music = Sound("file:///Users/yuxianghuang/desktop-cmu-graphics/background.mp3")
sound_effect = Sound("file:///Users/yuxianghuang/desktop-cmu-graphics/se1.wav")

background_music.play()


def onMousePress(mouseX, mouseY):
    sound_effect.play()

cmu_graphics.run()

@Mohamed-Waiel-Shikfa
Copy link
Contributor Author

Mohamed-Waiel-Shikfa commented Jul 31, 2024 via email

@Mohamed-Waiel-Shikfa
Copy link
Contributor Author

@Yuxiang-Huang YES! you are right.
My implementation did not allow for multiple sound being played at the same time. Nevertheless, I looked into it, and it should now be fixed.
As a bonus I also added a quick way to change the volume of individual sounds.

Hope this now is good enough to be merged into main :)

@schmave
Copy link
Member

schmave commented Aug 26, 2024

Thanks, @Mohamed-Waiel-Shikfa! I hope to review this again this week.

@Mohamed-Waiel-Shikfa
Copy link
Contributor Author

So... any news? Perhaps a comment on the code?

Also, just wanted to update you—I'm also a CA for 15112 this semester. I've talked with Professor Riley about using 1 to 2 hours of my paid time each week to work on cmu_graphics. If you have anything new you want me to focus on, let me know; otherwise, I'll keep going as usual.

Copy link
Member

@schmave schmave left a comment

Choose a reason for hiding this comment

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

This is a big improvement!

However, this code doesn't run correctly:

CMU_GRAPHICS_DEBUG = True

from cmu_graphics import *

plastic = Sound("plastic.mp3")
timer = Sound('timer.wav')



def onMousePress(x, y):
    plastic.play(loop=False, restart=True)


def onKeyPress(key):
    if key == 'a':
        timer.play(restart=True, loop=False)

cmu_graphics.loop()

Here are the sound files I used:
test-sounds.zip

To reproduce the bug:

  1. Click on the canvas and hear the plastic sound play all the way to the end (4-5 seconds).
  2. Type the letter 'a' and hear the timer sound start.
  3. Click on the canvas while the timer sound is still playing. You will hear the plastic sound start and the timer immediately stop. I expected that the timer sound would continue to play.

cmu_graphics/cmu_graphics.py Outdated Show resolved Hide resolved
cmu_graphics/cmu_graphics.py Outdated Show resolved Hide resolved
cmu_graphics/cmu_graphics.py Outdated Show resolved Hide resolved
@Mohamed-Waiel-Shikfa
Copy link
Contributor Author

I fixed the reported bug, thanks for the catch!

@Mohamed-Waiel-Shikfa
Copy link
Contributor Author

@schmave Could you take another look at this now?

@linxuanm
Copy link
Contributor

linxuanm commented Nov 8, 2024

Hi! Thank you for your contribution :)
I was assigned to this PR, and will take a look at the changes ASAP!

Edit: all code behaviors seem good after testing locally! I think this is ready to be merged after a bit of cleanup/styling!

raise Exception(err)

Sound.number_of_sounds += 1
if pygame.mixer.get_num_channels()==Sound.number_of_sounds:
Copy link
Contributor

Choose a reason for hiding this comment

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

Request change: code style (whitespace around operators)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +232 to +241
for i in range(10):
try:
response = webrequest.get(url)
self.sound = io.BytesIO(response.read())
except:
if i<9:
continue
else:
raise Exception('Failed to load sound data')
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Request change: I don't think this is a good way to handle failed requests; something like the image fetching logic might be more appropriate here (also, pyThrow might be preferable over generic Python error types).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rationale behind this code is to make the cmu_graphics more robust; if a network error happens, we don't just want to directly crash the program and instead try again a couple of times. In this spirit, I actually think that the sound fetching logic should be updated to reflect the behaviour seen here

pyThrow isn't defined in cmu_graphics.py nor was it ever used there; instead, normal exception was used. pyThrow is only defined inside shape_logic.py and is merely a simple wrapper of the exception class. We could do something about this, but if we are going to, we need to be consistent with it. So, any new thoughts on this?

@schmave schmave merged commit 6ee102a into cmu-cs-academy:main Nov 14, 2024
1 check was pending
@schmave
Copy link
Member

schmave commented Nov 14, 2024

This has been deployed to PYPI as version 1.1.37: https://pypi.org/project/cmu-graphics/ It's also available via the zip download link at https://academy.cs.cmu.edu/desktop

Thanks for all your work on this @Mohamed-Waiel-Shikfa !!

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.

4 participants