ALTTP: Add "oof" sound customization option #709

Nyx-Edelstein · 2022-06-25T19:10:52Z

This change adds the ability to swap Link's "oof" sound effect with one the user provides.

The required format for this sound effect is a .brr file no more than 2673 bytes. This can be generated from a 16-bit signed PCM (.wav) at 12khz and no longer than 0.394 seconds, using the snesbrr tool (https://github.com/boldowa/snesbrr). The .wav file can be obtained using a tool like Audacity by editing an existing sample or recording a custom one.

Sample test file (female "oof" sound): https://github.com/Nyx-Edelstein/The-Unachievable-Ideal-of-Chibi-Elf-Grunting-Noises-When-They-Get-Punched-A-Z3-Rom-Patcher/blob/main/z3_oof_patcher/default.brr

Example CLI usage:

.\snesbrr.exe -e .\oof.wav oof.brr
python .\LttPAdjuster.py --baserom .\baserom.sfc --oof .\oof.brr .\romtobeadjusted.sfc

The gui option simply prompts the user to select a .brr file and ensures it is the right file size.

Original PR from krelbel's branch: #502

Creating a new PR from personal fork

Ijwu · 2022-06-28T00:53:00Z

Code looks good but I do want to pull this and test it... or have someone else do it.

worlds/alttp/Rom.py

Include 'oof' argument in call to apply_rom_settings

Nyx-Edelstein · 2022-06-28T20:48:26Z

Should the linter error be addressed? No one commented on it and I didn't look too closely at krelbel's code.

alwaysintreble · 2022-06-29T00:53:22Z

I didn't have time to fully review the code earlier but now that I have it's fine. The only things that stick out to me are Rom.py 1810 and 501 in lttpadjuster is a docstring so should """look like this""". @Ijwu may have opinions on that tooltips class at 1238 as he really despises LGPL licenses though and that alone may honestly not get approved...

Ijwu · 2022-06-29T01:52:41Z

@Ijwu may have opinions on that tooltips class at 1238 as he really despises LGPL licenses though and that alone may honestly not get approved...

I'm not seeing how lgpl factors in here. Where do you see it?

alwaysintreble · 2022-06-29T02:01:57Z

looks like the source i thought was the wcktooltips source was actually just using it and that was under lgpl. that's my b 😳

Nyx-Edelstein · 2022-06-29T02:02:15Z

It was taken from an example on Stack Overflow so its definitely not lgpl. Technically it would be under CC BY-SA 4.0 (https://creativecommons.org/licenses/by-sa/4.0/) which requires attribution, but I have literally never seen anyone care about that, especially not in a noncommercial project, as most people posting to Stack Overflow do so with the intention of releasing it to public domain.

Ijwu · 2022-06-29T02:05:03Z

It was taken from an example on Stack Overflow so its definitely not lgpl. Technically it would be under CC BY-SA 4.0 (https://creativecommons.org/licenses/by-sa/4.0/) which requires attribution, but I have literally never seen anyone care about that, especially not in a noncommercial project, as most people posting to Stack Overflow do so with the intention of releasing it to public domain.

Please attribute the author of the SO post and link to it in a comment in the code. Thank you.

Yes, the linter error should be addressed. This won't get merged until it does just due to the checks we have in place.

Attribution

Berserker66 · 2022-06-29T02:10:50Z

We have other parts of code that are yoinked from stackoverflow, typically we permalink to the post containing the code, so that's the least that should be done here too then. The linter error points out that if your code ever makes it to the error case the logger instruction will result in a crash as the name was never defined, so that needs fixing; yes.

Nyx-Edelstein · 2022-06-29T02:19:08Z

Sure thing, link to post added.

Remove unnecessary code / linter error

Berserker66 · 2022-06-29T02:31:01Z

removing the check isn't exactly a great fix ;)

CLI file size guard

Nyx-Edelstein · 2022-06-29T02:41:03Z

removing the check isn't exactly a great fix ;)

Still intending to add the tutorial as per @alwaysintreble's comments above, will move out of draft after it's completed

strotlog · 2022-06-29T19:04:00Z

Sure thing, link to post added.

Stack Overflow states that contributions made on the date of that 2019-2020 contribution are governed by https://creativecommons.org/licenses/by-sa/4.0/ (Stack Overflow's link), that link gives in part of its non-legal summary:

Attribution — You must give appropriate credit, provide a link to the license, and indicate if changes were made. You may do so in any reasonable manner, but not in any way that suggests the licensor endorses you or your use.

ShareAlike — If you remix, transform, or build upon the material, you must distribute your contributions under the same license as the original.

"Same license" links to some other compatible licenses you can use instead, but MIT is not noted as being one of them.

strotlog · 2022-06-29T19:09:52Z

Oops, I meant to come in here just to suggest that the PR and/or final squash commit title should have the game's name for context.

strotlog · 2022-06-29T19:17:41Z

Sorry for commenting so many times. Just noticed that one of the comments under the StackOverflow contribution that we want to use actually calls out that it does not appear to be an original work, and links to https://web.archive.org/web/20180806215516/http://www.voidspace.org.uk/python/weblog/arch_d7_2006_07_01.shtml as the possible original.

Berserker66 · 2022-09-17T00:22:55Z

Unfortunately in testing this Enemizer fails the required version check due to version tag error in Enemizer, reported issue in Ijwu/Enemizer#15

Nyx-Edelstein · 2022-09-18T00:00:58Z

Unfortunately in testing this Enemizer fails the required version check due to version tag error in Enemizer, reported issue in Ijwu/Enemizer#15

Anything you need me to do?

Berserker66 · 2022-09-18T00:50:18Z

No, the ball is back in @Ijwu 's court.

alwaysintreble · 2022-09-24T19:31:27Z

Grabbed the latest Enemizer release and it's working correctly for me now. GUI element and all. wouldn't hurt to have someone else verify though.

alwaysintreble · 2022-09-24T19:36:43Z

worlds/alttp/__init__.py

@@ -388,6 +397,7 @@ def generate_output(self, output_directory: str):
                               world.menuspeed[player].current_key,
                               world.music[player],
                               world.sprite[player],
+                               world.oof[player],


actually just had a generation issue since this attribute doesn't exist at run time. is this used by the adjuster or do we rip this out until implementing as a generation option?

I don't think it does. I just removed it.

ThePhar · 2022-10-28T22:18:28Z

So this still only plays the "meme" sound when enemy shuffle is on, even with Enemizer 7.1 installed. Works fine without Enemizer active though.

Nyx-Edelstein · 2022-11-10T19:45:26Z

So this still only plays the "meme" sound when enemy shuffle is on, even with Enemizer 7.1 installed. Works fine without Enemizer active though.

I've been unable to get the latest build running due to not being able to find some required modules, but I can't imagine that the correct version of Enemizer is being used for AP. I did however test it by manually patching a rom generated with enemizer and it worked correctly.

ThePhar · 2022-11-11T23:25:02Z

So this still only plays the "meme" sound when enemy shuffle is on, even with Enemizer 7.1 installed. Works fine without Enemizer active though.

I've been unable to get the latest build running due to not being able to find some required modules, but I can't imagine that the correct version of Enemizer is being used for AP. I did however test it by manually patching a rom generated with enemizer and it worked correctly.

I generated locally with a fresh clone of this AP PR and latest version of Enemizer (7.1), so if you could verify it working with the integrated Enemizer during generation, that would be excellent.

Nyx-Edelstein · 2023-02-13T21:05:58Z

@ThePhar I'm not sure why my enemizer fix isn't being applied. There should be no way for it to be included. Is there some other step for repatching the ASM files that isn't being done in the build process?

Regardless, I readded my previous fix for just overwriting the relevant bytes with what they should be as part of the Oof SFX patch. I understand why this is dispreferred as per Berserker66 above but in this case does it really matter? This is unused functionality of Enemizer that nothing else touches and which (I am told from VT developers) shouldn't even be there.

Co-authored-by: Fabian Dill <Berserker66@users.noreply.github.com> Co-authored-by: Fabian Dill <fabian.dill@web.de> Co-authored-by: Zach Parks <zach@alliware.com>

Nyx-Edelstein added 4 commits June 19, 2022 11:47

Adding CLI Oof Sounds from Krebel's branch

7bd9f24

GUI code for selecting "OOF" sounds

1ae878e

Merge branch 'ArchipelagoMW:main' into main

2a44092

Merge branch 'ArchipelagoMW:main' into main

ec69a3f

alwaysintreble reviewed Jun 28, 2022

View reviewed changes

worlds/alttp/Rom.py Show resolved Hide resolved

Update __init__.py

3b03ee6

Include 'oof' argument in call to apply_rom_settings

Update LttPAdjuster.py

f03c97a

Attribution

Update Rom.py

1228fcb

Remove unnecessary code / linter error

Nyx-Edelstein added 2 commits June 28, 2022 19:33

Update LttPAdjuster.py

379641e

CLI file size guard

Update LttPAdjuster.py

8150162

Nyx-Edelstein marked this pull request as draft June 29, 2022 02:39

Nyx-Edelstein added 2 commits June 28, 2022 19:55

Update LttPAdjuster.py

40d8f19

Create tutorial for replacing oof sounds

634455a

Nyx-Edelstein marked this pull request as ready for review June 29, 2022 04:31

Merge branch 'main' into main

ee68c6c

Nyx-Edelstein changed the title ~~Add "oof" sound customization option~~ ALTTP: Add "oof" sound customization option Jun 29, 2022

Berserker66 mentioned this pull request Sep 17, 2022

Wrong Version tag Ijwu/Enemizer#15

Open

Check for Enemizer version

a32a965

alwaysintreble reviewed Sep 24, 2022

View reviewed changes

Nyx-Edelstein added 2 commits October 4, 2022 22:44

Merge branch 'main' into main

882e5ef

Update __init__.py

a334e09

Nyx-Edelstein requested review from alwaysintreble and Ijwu and removed request for alwaysintreble and Ijwu October 5, 2022 05:48

ThePhar added 2 commits October 28, 2022 15:34

Merge branch 'main' into main

a86f352

Fix missing argument in apply_rom_settings preventing generation.

883008e

Merge branch 'main' into main

3946a1d

ThePhar self-assigned this Nov 1, 2022

Nyx-Edelstein added 2 commits February 13, 2023 12:23

Merge branch 'main' into main

27415db

Update Rom.py

26c286c

Merge branch 'main' into main

50187a3

ThePhar approved these changes Apr 11, 2023

View reviewed changes

ThePhar merged commit 8b7ffaf into ArchipelagoMW:main Apr 11, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ALTTP: Add "oof" sound customization option #709

ALTTP: Add "oof" sound customization option #709

Nyx-Edelstein commented Jun 25, 2022

Ijwu commented Jun 28, 2022

Nyx-Edelstein commented Jun 28, 2022

alwaysintreble commented Jun 29, 2022

Ijwu commented Jun 29, 2022

alwaysintreble commented Jun 29, 2022 •

edited

Nyx-Edelstein commented Jun 29, 2022

Ijwu commented Jun 29, 2022

Berserker66 commented Jun 29, 2022 •

edited

Nyx-Edelstein commented Jun 29, 2022 •

edited

Berserker66 commented Jun 29, 2022

Nyx-Edelstein commented Jun 29, 2022

strotlog commented Jun 29, 2022

strotlog commented Jun 29, 2022

strotlog commented Jun 29, 2022

Berserker66 commented Sep 17, 2022

Nyx-Edelstein commented Sep 18, 2022

Berserker66 commented Sep 18, 2022

alwaysintreble commented Sep 24, 2022

alwaysintreble Sep 24, 2022

Nyx-Edelstein Oct 5, 2022

ThePhar commented Oct 28, 2022

Nyx-Edelstein commented Nov 10, 2022

ThePhar commented Nov 11, 2022

Nyx-Edelstein commented Feb 13, 2023

ALTTP: Add "oof" sound customization option #709

ALTTP: Add "oof" sound customization option #709

Conversation

Nyx-Edelstein commented Jun 25, 2022

Ijwu commented Jun 28, 2022

Nyx-Edelstein commented Jun 28, 2022

alwaysintreble commented Jun 29, 2022

Ijwu commented Jun 29, 2022

alwaysintreble commented Jun 29, 2022 • edited

Nyx-Edelstein commented Jun 29, 2022

Ijwu commented Jun 29, 2022

Berserker66 commented Jun 29, 2022 • edited

Nyx-Edelstein commented Jun 29, 2022 • edited

Berserker66 commented Jun 29, 2022

Nyx-Edelstein commented Jun 29, 2022

strotlog commented Jun 29, 2022

strotlog commented Jun 29, 2022

strotlog commented Jun 29, 2022

Berserker66 commented Sep 17, 2022

Nyx-Edelstein commented Sep 18, 2022

Berserker66 commented Sep 18, 2022

alwaysintreble commented Sep 24, 2022

alwaysintreble Sep 24, 2022

Choose a reason for hiding this comment

Nyx-Edelstein Oct 5, 2022

Choose a reason for hiding this comment

ThePhar commented Oct 28, 2022

Nyx-Edelstein commented Nov 10, 2022

ThePhar commented Nov 11, 2022

Nyx-Edelstein commented Feb 13, 2023

alwaysintreble commented Jun 29, 2022 •

edited

Berserker66 commented Jun 29, 2022 •

edited

Nyx-Edelstein commented Jun 29, 2022 •

edited