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

Add sound effects #108

Merged
merged 32 commits into from
Jun 5, 2024
Merged

Conversation

Suprapote
Copy link
Contributor

@Suprapote Suprapote commented May 25, 2024

Description

Adding sound for make the navigation not so borring.

Motivation and Context

This issue

How Has This Been Tested?

Screenshots

MKV.to.MP4.Converter.-.FreeConvert.com.mp4

Types of changes

  • Improvement (non-breaking change that adds a new feature)
  • Bug fix (fixes an issue)
  • Breaking change (breaking change)
  • Config and build (change in the configuration and build system, has no impact on code or features)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>

@Suprapote
Copy link
Contributor Author

Suprapote commented May 26, 2024

PD: I changed the cursor sound

Copy link
Owner

@Polprzewodnikowy Polprzewodnikowy left a comment

Choose a reason for hiding this comment

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

Overall really good work. I have some nitpicks to make code look cleaner.

One more question: under what license are these sound effects?

src/menu/components/context_menu.c Outdated Show resolved Hide resolved
src/menu/menu.c Outdated
Comment on lines 72 to 78
mixer_ch_set_vol(SFX_CHANNEL, 0.5f, 0.5f);
wav64_open(&sfx_cursor, "rom:/cursorsound.wav64");
wav64_open(&sfx_exit, "rom:/back.wav64");
wav64_open(&sfx_settings, "rom:/settings.wav64");
wav64_open(&sfx_enter, "rom:/enter.wav64");
wav64_open(&sfx_error, "rom:/error.wav64");

Copy link
Owner

Choose a reason for hiding this comment

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

This can be moved into sound_reconfigure. Don't forget about a flag to do loading only once.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have done what I can to make this better, but might be a better way. Please let us know.

src/menu/menu.c Outdated Show resolved Hide resolved
src/menu/menu.h Outdated Show resolved Hide resolved
src/menu/menu.h Outdated Show resolved Hide resolved
@Suprapote
Copy link
Contributor Author

Suprapote commented May 28, 2024

I extracted the sounds from the web that networkfusion recommended. I readed the rules and apparently all sounds in that page are free. If you want to check it out: https://pixabay.com/es/service/license-summary/

@networkfusion
Copy link
Collaborator

Nice work. I would suggest also updating the readme with the "notable feature" list and adding the source of the wav files (and their names/creator) to the "Open source software and licenses used" list.

@networkfusion
Copy link
Collaborator

I have been messing around with this, but not tested... My current state is https://github.com/networkfusion/N64FlashcartMenu/tree/refactor-sfx. Feel free to use it!

@networkfusion
Copy link
Collaborator

networkfusion commented Jun 5, 2024

@Suprapote here are a few more changes to complete the refactor: Suprapote#1

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Suprapote
Copy link
Contributor Author

I deleted the last two because I extracted them from the actions sound

@networkfusion
Copy link
Collaborator

I deleted the last two because I extracted them from the actions sound

In that case, probably better to update the action sound entry to state it is also for the other ones used.

@networkfusion
Copy link
Collaborator

I deleted the last two because I extracted them from the actions sound

In that case, probably better to update the action sound entry to state it is also for the other ones used.

Actually, after re-reading, probably fine.

README.md Outdated Show resolved Hide resolved
src/menu/menu.c Outdated Show resolved Hide resolved
src/menu/menu.h Outdated Show resolved Hide resolved
src/menu/sound.c Outdated Show resolved Hide resolved
src/menu/sound.c Outdated Show resolved Hide resolved
@networkfusion
Copy link
Collaborator

Okay, I think I have done all I can to help with this PR 😅 . Hopefully my help has been useful and it helps understanding of the codebase and what is expected by your many future PR's @Suprapote 😁 .

@Suprapote
Copy link
Contributor Author

Yes, thank you for your help 😁

@Suprapote
Copy link
Contributor Author

Final result:

2024-06-05.20-12-01.mp4

Comment on lines +118 to +120
if (menu->settings.sound_enabled) {
sound_init_sfx();
}
Copy link
Owner

Choose a reason for hiding this comment

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

Minor nitpick because changing this setting live will not enable sounds and will require complete reboot of the menu. It's okay for now because we don't have a settings editor yet.

@Polprzewodnikowy Polprzewodnikowy changed the base branch from main to develop June 5, 2024 20:03
@Polprzewodnikowy Polprzewodnikowy merged commit 3bc0ac5 into Polprzewodnikowy:develop Jun 5, 2024
2 checks passed
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.

3 participants