(Improvement) Implement playing sound by string ID #5201

JeBobs · 2025-07-21T01:15:38Z

I've replaced enum-based sound playing events with string-based equivalents, which should open the door for server customization and other enhancements in the future

Added SoundLookup class with different registry lookup methods depending on server version.
Added the ability to configure what sounds are played depending on event, with a fallback built into SoundType.
Removed getCrippleSound as SoundLookup can now fall back to the original default sound if the mace sound doesn't exist on the server's Minecraft version.
Added a EnableCustomSounds config variable that will skip SoundLookup ID checking and just pass the sound string directly to the client, mainly due to the fact that it isn't possible to verify if resource pack values exist.
Cleaned up a few switch statements to match how the original getSound had it formatted.

I'd love to see/do a further expansion of sound configuration for each ability now that we can just fall back to generic, but that may be for another PR.

I've replaced enum-based sound playing events with string-based equivalents, which should open the door for server customization and other enhancements in the future - Added SoundLookup class with different registry lookup methods depending on server version. - Added the ability to configure what sounds are played depending on event, with a fallback built into SoundType. - Removed getCrippleSound as SoundLookup can now fall back to the original default sound if the mace sound doesn't exist on the server's Minecraft version. - Added a EnableCustomSounds config variable that will skip SoundLookup ID checking and just pass the sound string directly to the client, mainly due to the fact that it isn't possible to verify if resource pack values exist. - Cleaned up a few switch statements to match how the original getSound had it formatted. I'd love to see/do a further expansion of sound configuration for each ability now that we can just fall back to generic, but that may be for another PR.

nossr50 · 2025-08-02T19:52:50Z

@JeBobs have you test on Spigot and Paper?
And Minecraft versions 1.18 -> Latest?

Copilot

Pull Request Overview

This PR replaces enum-based sound playing with string-based sound IDs to enable server customization and future enhancements. The implementation adds a sound lookup system that can handle different Minecraft server versions and provides fallback mechanisms for unsupported sounds.

Key changes:

Added string-based sound ID system with configurable sound mappings
Implemented SoundLookup class with version-aware registry checking
Added EnableCustomSounds configuration option for resource pack compatibility

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
sounds.yml	Adds sound ID mappings and EnableCustomSounds configuration option
SoundType.java	Converts enum to include default sound IDs and modernizes switch syntax
SoundManager.java	Replaces enum-based sound lookup with string-based system using SoundLookup
SoundLookup.java	New utility class for checking sound existence across different server versions
SoundConfig.java	Adds methods to retrieve sound IDs and custom sound configuration

Copilot · 2025-08-02T19:53:51Z

src/main/java/com/gmail/nossr50/util/sounds/SoundType.java

+    BLEED("minecraft:entity.ender_eye.death"),
+    GLASS("minecraft:block.glass.break"),
+    ITEM_CONSUMED("minecraft:item.bottle.empty"),
+    CRIPPLE("minecraft:block.anvil.place");


The CRIPPLE sound is using 'minecraft:block.anvil.place' as fallback, but based on the sounds.yml configuration, it should use 'minecraft:item.mace.smash_ground'. This inconsistency means the fallback won't match the configured sound ID.

Suggested change

CRIPPLE("minecraft:block.anvil.place");

CRIPPLE("minecraft:item.mace.smash_ground");

Copilot · 2025-08-02T19:53:52Z

src/main/java/com/gmail/nossr50/config/SoundConfig.java

+
+    public boolean getCustomSoundEnabled() {
+        String key = "Sounds.EnableCustomSounds";
+        return config.getBoolean(key, true);


The default value for EnableCustomSounds should be 'false' to match the configuration file default, not 'true'. This could cause unexpected behavior when the configuration is not explicitly set.

Suggested change

return config.getBoolean(key, true);

return config.getBoolean(key, false);

Copilot · 2025-08-02T19:53:52Z

src/main/java/com/gmail/nossr50/config/SoundConfig.java

+    }
+
    public boolean getIsEnabled(SoundType soundType) {
        String key = "Sounds." + soundType.toString() + ".Enabled";


The key construction uses '.Enabled' but the YAML configuration uses '.Enable' (without 'd'). This will cause the configuration lookup to fail and always return the default value.

Suggested change

String key = "Sounds." + soundType.toString() + ".Enabled";

String key = "Sounds." + soundType.toString() + ".Enable";

Copilot · 2025-08-02T19:53:52Z

src/main/java/com/gmail/nossr50/util/sounds/SoundLookup.java

+            Class<?> registry = Class.forName("org.bukkit.Registry");
+            Object soundReg   = registry.getField("SOUND_EVENT").get(null);
+            Method get        = soundReg.getClass().getMethod("get", NamespacedKey.class);
+            return get.invoke(soundReg, NamespacedKey.fromString(id)) != null;


NamespacedKey.fromString(id) can throw IllegalArgumentException for invalid namespaced keys. This should be wrapped in a try-catch block to prevent the exception from propagating and causing the method to return false incorrectly.

Suggested change

return get.invoke(soundReg, NamespacedKey.fromString(id)) != null;

NamespacedKey key;

try {

key = NamespacedKey.fromString(id);

} catch (IllegalArgumentException e) {

return false;

}

return get.invoke(soundReg, key) != null;

nossr50 · 2025-08-02T19:55:25Z

@JeBobs Don't mind the copilot comments too much, I saw the "request review from copilot" and was wondering what the heck it would even do, so I pressed it to "try it and see".

JeBobs · 2025-08-05T15:06:58Z

i tested spigot 1.20.4 and paper 1.21.4 as those are what I've had on hand, but I'll do more extensive testing later today

nossr50 · 2025-08-10T17:46:17Z

i tested spigot 1.20.4 and paper 1.21.4 as those are what I've had on hand, but I'll do more extensive testing later today

Mostly, I'd want you to test Spigot and Paper for the oldest supported version as well to make sure no exceptions are being thrown / etc.

As of right now, that would be Minecraft version 1.18.2

JeBobs · 2025-08-13T18:22:25Z

Ended up taking longer than I anticipated to be able to test, but after creating both Spigot and Paper 1.18.2 test environments, every sound event that I attempted to trigger seemed to work exactly as intended. I'll keep testing to see if there are any edge cases, but ability rankup, skill levelup, readying/using abilities all seem to play with no perceivable difference.

nossr50 · 2025-08-17T20:51:56Z

I appreciate you taking time for manual testing.

nossr50 · 2025-08-17T21:11:33Z

I'll make some tweaks to your branch and merge afterwards, might not get to it this weekend.

nossr50 · 2025-08-30T16:31:15Z

@JeBobs I'm making some optimizations now, reflection calls are expensive and should be cached. I'm also changing how custom sounds are defined, you'll see soon.

…eflection calls

nossr50 requested a review from Copilot August 2, 2025 19:53

Copilot AI reviewed Aug 2, 2025

View reviewed changes

Merge branch 'master' into sound_lookup

22f9a0f

Fix getIsEnabled using wrong key

2c202cb

nossr50 added 2 commits August 30, 2025 10:16

always use registry, simplify custom sound enabling logic, optimize r…

e89d44e

…eflection calls

forgot we need this for legacy versions

e51d00b

nossr50 merged commit df69410 into mcMMO-Dev:master Aug 30, 2025
1 check passed

	CRIPPLE("minecraft:block.anvil.place");
	CRIPPLE("minecraft:item.mace.smash_ground");

	return config.getBoolean(key, true);
	return config.getBoolean(key, false);

	String key = "Sounds." + soundType.toString() + ".Enabled";
	String key = "Sounds." + soundType.toString() + ".Enable";

-            return get.invoke(soundReg, NamespacedKey.fromString(id)) != null;
+            NamespacedKey key;
+            try {
+                key = NamespacedKey.fromString(id);
+            } catch (IllegalArgumentException e) {
+                return false;
+            }
+            return get.invoke(soundReg, key) != null;

Uh oh!

(Improvement) Implement playing sound by string ID #5201

(Improvement) Implement playing sound by string ID #5201

Uh oh!

Conversation

JeBobs commented Jul 21, 2025

Uh oh!

nossr50 commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

nossr50 commented Aug 2, 2025

Uh oh!

JeBobs commented Aug 5, 2025

Uh oh!

nossr50 commented Aug 10, 2025

Uh oh!

JeBobs commented Aug 13, 2025

Uh oh!

nossr50 commented Aug 17, 2025

Uh oh!

nossr50 commented Aug 17, 2025

Uh oh!

nossr50 commented Aug 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nossr50 commented Aug 2, 2025 •

edited

Loading