NeoForge icon indicating copy to clipboard operation
NeoForge copied to clipboard

[1.20.5/6] Select Music Event

Open Zepalesque opened this issue 2 months ago • 14 comments

What this does is adds an event for when Minecraft#getSituationalMusic() gets fired, allowing for modders to change/set the music based on whatever conditions they want (that are available from the client of course)

hopefully I did everything right, this is my first ever pull reqeust for either forge or neoforge, but I did follow the contributing instructions so hopefully it should be good

Zepalesque avatar Apr 26 '24 13:04 Zepalesque

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 26 '24 13:04 CLAassistant

  • [ ] Publish PR to GitHub Packages

ah yeah, good idea, hadn’t thought of that

Zepalesque avatar Apr 26 '24 17:04 Zepalesque

ok, changes made

Zepalesque avatar Apr 26 '24 17:04 Zepalesque

ah yeah good idea, I’ll do this when get the chance

Zepalesque avatar Apr 26 '24 19:04 Zepalesque

Honestly, I have no idea how this will work in correlation to multiple mods. For example, if a mod played situational music based upon being in a given structure while a different mod played situational music based on an item the player was wearing on their head, which one would take precedence?

I'm not against the PR, but I think there should be some ordering that should be either defined or explained with some custom system or using the priority of the event listener (e.g., HIGHEST for helmet, LOW for in a structure, LOWEST for a biome/dimension. Other ideas are welcome.

Also, I think the music that should be present in the event is the current music, not the one that Mojang is going to take over after yours.

ChampionAsh5357 avatar Apr 26 '24 19:04 ChampionAsh5357

hmm yeah I can probably add some documentation stuff for the event priority stuff, good idea

for the other part, I’m not 100% sure what you mean, but if you mean that you should be able to change the currentMusic field, then there are a few reasons I didn’t do that, mainly because letting you set that could break stuff since that’s just used to track the current music, so changing it would likely lead to things like overlapping music tracks, plus you can access it if necessary for some reason or another anyway through Minecraft.getInstance().musicManager.currentMusic

You can also still have the music track instantly start playing as well and stop existing music, if that’s what you meant, you just have to set the Music so that replaceCurrentMusic() returns true (can’t remember the exact field name atm)

The specific use case this was initially meant for was for the Aether mod, which overrides creative music in its biomes so that it plays the biome music instead, plus one of its addons that additionally adds nighttime music tracks for the dimension These are supposed to not instantly replace the existing music, but instead they are supposed to play once the previous music is done and the delay timer ends (for the nighttime music tracks at least, the others are just made the default for the dimension of that makes sense), similar to vanilla music, just allowing a more dynamic system, while also allowing for there to still be other uses

Previously the Aether had used it’s own entirely new music manager, but I figured out that it wouldn’t really be necessary to have a separate one if there was a way to modify what music gets chosen based on the situation

heck this got long, whoops lol, hopefully it’s not too convoluted :joy:

Zepalesque avatar Apr 26 '24 20:04 Zepalesque

I don't mean you should change the currentMusic field, rather that the information makes more sense in terms of the event context. The situational music from Mojang just indicates what the default music would be playing. If we know the event logic will play its own sound, it might not make sense to execute Minecraft#getSituationalMusic. The current music is more useful as it indicates what the custom music will be overriding if it chooses to do so.

ChampionAsh5357 avatar Apr 26 '24 20:04 ChampionAsh5357

The issue with that is that if you wanted to somehow utilize the original default music track for the situation when choosing the new potential music track, you wouldn’t be able to if it instead gave you the current playing music (for instance, using a map to switch to a different music track if a certain one would’ve been played plus a certain condition, such as it being nighttime) Not only that, but it could also cause discrepancy between the situation and the music, as if you used the current music for the same purpose instead, that might not be the default one based on the current situation, and it would give the incorrect music it’s also meant to be able to be chained together, so that if you have the event fire after another mod’s event you are given both the original music and the one that the other mod set it to

pretty much, the reason it calls Minecraft#getSituationalMusic is so that it can be used as a variable for the modder when they are choosing the music they want to have chosen instead hopefully that makes sense and isn’t just a big jumble of word soup

Zepalesque avatar Apr 26 '24 20:04 Zepalesque

Note for my Bumblezone mod, I have a structure that should stop all other music when player is in it and plays the structure’s own music. Then there is an arena event in the structure that can be triggered that will stop the structure music and play the arena event music.

Which means my use case is one where I’m going to want to override everyone else’s music. At least to me, I feel like I want high priority. Is there a reason why a helmet music should override my event music?

TelepathicGrunt avatar Apr 26 '24 20:04 TelepathicGrunt

most likely what you’d want to do is have the structure music be high priority, and then the event music higher priority, and have both replaceCurrentMusic not sure what you mean by helmet music tho

Zepalesque avatar Apr 26 '24 20:04 Zepalesque

pretty much, the reason it calls Minecraft#getSituationalMusic is so that it can be used as a variable for the modder when they are choosing the music they want to have chosen instead

I understand, though I feel as though Minecraft#getSituationalMusic also falls into that category as whatever it outputs would be the currently playing music, but eh. If you believe that it doesn't make sense, I'll leave it at that since I don't have a solid enough argument for or against it.

Which means my use case is one where I’m going to want to override everyone else’s music. At least to me, I feel like I want high priority. Is there a reason why a helmet music should override my event music?

My general thought process was that things closer to the player in context would take higher priority, since that would be what the player is focusing on. However, it's up to the modders to determine which priority makes sense for their mod. I was just using the helmet as an example.

ChampionAsh5357 avatar Apr 26 '24 20:04 ChampionAsh5357

ah ok I see, I didn’t see the part about having an item on the players head before lol so I was confused when telepathicgrunt mentioned helmets lol

but yeah music keeps playing even if it’s not the one that would’ve been picked, so if you are in one biome with one music, which is playing, and you go to another with different music then it won’t just instantly stop the music so it won’t necessarily be the same

hmm, semi-unrelated to this specific conversation, but I just realized an issue that changing it so it reassigns the music variable rather than having a separate posted variable would resolve, so that’s a good thing

Zepalesque avatar Apr 26 '24 20:04 Zepalesque

ok, changed it so it reassigns the music variable along with having more in-depth documentation

Zepalesque avatar Apr 27 '24 01:04 Zepalesque