-
Notifications
You must be signed in to change notification settings - Fork 36
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
Feature #30
base: master
Are you sure you want to change the base?
Feature #30
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this contribution, this is really awesome, I'm sure my youngest one will appreciate the nice animals :-)
It would be great if you could make a few changes:
- please make sure the checks pass
- please try to ensure that individual commits are well-formed, i.e. please squash the last cleanup one with the first one
- please address the comments inline
I also noticed that images no longer display for me when I run the program without installation, from the current directory. How about yourself, does it still work?
I'm also wondering why you decided to get rid of the old images and sounds? Some of them I do not miss, but there were some which perhaps could make sense when showing letters for example?
Finally, we need to take care of some legal considerations to make it possible to distribute the new files in Debian. Please briefly describe (in the commit message or PR description/comment is fine) who is the copyright holder for each of them, what is the time of their initial creation, and what is the license.
if event.type == KEYDOWN and self.args.deterministic_sounds: | ||
self.sounds[event.key % len(self.sounds)].play() | ||
if event.type == pygame.KEYDOWN: | ||
# show self.images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this comment, it makes even less sense now.
@@ -0,0 +1,4 @@ | |||
[Dolphin] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file for? I don't think bambam
uses it? In that case please add it to .gitignore
and remove from PR.
FWIW, I seem to have found the source for one of the images, and it is in public domain, so perfectly acceptable. Unfortunately I do not have the time to track down all the other files, but I would gladly review any such contribution. |
Seems to me this PR should be split up between the code changes that allow more control over pairing of deterministic sounds and images (and actually, what about deterministic pairing while retaining random non-deterministic keys?) and the new image files. Images and sounds are a separate type of thing than the code changes that support different functions. |
I agree, good point. There is some more great rationale in your group post.
Can you please clarify what exactly you mean here @wolftune ? |
I mean reliably play the same-named sounds with images (like sheep sound with sheep image) without deterministically setting which key goes with which image. Obviously for letters, it would have to be deterministic (and actually sounds for letters deterministically would need a different code because what's being matched isn't images but characters) |
I gave this some thought and came up with the following design draft. For each keypress event, a sound and a picture is selected by consulting There would be a few generic policies:
A few sound-specific selection policies:
A few image-specific selection policies:
Using the above, it should be possible to implement at least the following use cases:
Please let me know what you think @wolftune . |
Sounds good to me, I haven't thought it through that deeply… |
@wolftune it took much longer than anticipated, but I finally pushed experimental support for extensions. Please give it a spin and let me know what you think. |
@porridge wow, neat! The updated README has a minor error where in the mention of which zip to download, "or" is included in the ticked codeblock. More significantly, it mentions the 1.3 release, but that isn't showing in the releases yet. I've not built the code before. If a functional binary is easily enough available, I'm happy to try it sometime soon. |
Thanks, I'll take a look.
Yes, it's prepared but I haven't cut it yet.
Please try https://github.com/porridge/bambam/archive/refs/heads/master.zip |
I'm not figuring out how to run it from that zip. I tried the various .desktop files and didn't get it to work. Maybe lacking the |
Please do open an issue and provide any output you got in the terminal, and how it failed. |
The -d (deterministic sound) parameter functionality has been changed.
I replaced the image and sound arrays to dictionaries with filenames (without extensions) as keys.
Matching filenames are played simultaneously.