VazkiiModsArchive / Ambience

Your music in minecraft
25 stars 49 forks source link

Allow multiple randomly-selected songs for an event #20

Closed Flashfyre closed 7 years ago

Flashfyre commented 7 years ago

I made this modification mostly for my own use but I saw someone else requesting this same feature so I made it a pull request. I may not have chosen the most efficient method to add this functionality so, if you choose to use this pull request, you may want to make your own adjustments before adding it. I worked out all the bugs I could find but there may be minor bugs present that I haven't found.

Commit Description:

Multiple song names can be provided for each event by separating the song names with a comma ',' in the properties file. When a song for the event should load, it will randomly load one of the songs but it will not play the same song twice in a row.

NOTE: This commit introduces a new song name limitation that song names should not contain the exact names of any other song names as this will cause conflicts.

Flashfyre commented 7 years ago

Bug that I found: Next Song text in debug doesn't disappear when going to a different event then going back to the first one. It also seems to be more insistent on changing the song when going to a different event (allows less time than previously) and does not return to the first song as easily. If I fix this, I will also post that here.

Edit: It turned out to be an easy fix. I just had to remove a condition from the old code.

Flashfyre commented 7 years ago

Another bug: Music does not resume after turning off the volume until a new event is triggered.

Edit: Fixed.

Flashfyre commented 7 years ago

Bug: Sometimes, when the song transitions to a different song, the song transitions out just after starting and starts again before playing all the way through.

Namnodorel commented 7 years ago

Thank you, I needed those features/bug fixes! Please merge it @Vazkii until then I'll use the fork of Samuel-Harbord

Vazkii commented 7 years ago

When I find some time I need to redo ambience from scratch. Until then I really don't have the time to maintain it. You're more than welcome to use the fork.

Flashfyre commented 7 years ago

@Namnodorel Here is a build if you haven't already done it yourself. http://www.mediafire.com/download/s1cswgzptrmbih4/Ambience_1.0-8.jar

Namnodorel commented 7 years ago

Oh thank you! I have actually tried to build it already, but ran into some issues, probably because I don't have anything set up for developing mods. I'd have looked further into it tomorrow, but if you have a build ready that's even better than me messing with other peoples code :D

Namnodorel commented 7 years ago

Uhm, that songpicking doesn't work as well as I thought...

java.lang.NullPointerException: Unexpected error at vazkii.ambience.SongPicker.getRandomSong(SongPicker.java:240) at vazkii.ambience.Ambience.onTick(Ambience.java:81) at net.minecraftforge.fml.common.eventhandler.ASMEventHandler_8_Ambience_onTick_ClientTickEvent.invoke(.dynamic) at net.minecraftforge.fml.common.eventhandler.ASMEventHandler.invoke(ASMEventHandler.java:90) at net.minecraftforge.fml.common.eventhandler.EventBus.post(EventBus.java:185) at net.minecraftforge.fml.common.FMLCommonHandler.onPostClientTick(FMLCommonHandler.java:344) at net.minecraft.client.Minecraft.func_71407_l(Minecraft.java:1851) at net.minecraft.client.Minecraft.func_71411_J(Minecraft.java:1055) at net.minecraft.client.Minecraft.func_99999_d(Minecraft.java:371) at net.minecraft.client.main.Main.main(SourceFile:124) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) at java.lang.reflect.Method.invoke(Unknown Source) at net.minecraft.launchwrapper.Launch.launch(Launch.java:135) at net.minecraft.launchwrapper.Launch.main(Launch.java:28)

Where my ambience.properties is just `# Ambience Config

enabled=true

event.generic=IntoTheMindsEye,TheJourneyHome `

This does work, if I define event.mainMenu, which should not be necessary

Also sorry for posting this here, but issues aren't enabled on the fork...

Flashfyre commented 7 years ago

Issues are enabled now so you can take this issue there. Also, if you could confirm that this only happens on my fork and not the original, that would be very helpful.

Flashfyre commented 7 years ago

I guess you won't have to, I fixed it. Here is an updated build: http://www.mediafire.com/file/m8k0ju35x1v1mzl/Ambience_1.0-9.jar

Iteries commented 7 years ago

Only thing I see after all this = Update to curse forge for downloading and reflect it on the code for bases.. ie the config for how-to add your own music modifications. I have some old Dungeon Keeper music/quotes like to add-to music grouping that this update offers. THANK YOU Namnodorel & Samuel for all your hard work and time.

P3rf3ctXZer0 commented 7 years ago

I too am testing this. Um I had it transition songs and just at the point of transition it play 3 secs of the song stopped for 2 secs and played the song all the way. Is that intended? Can I add something the config that removes transition fade?

I hope this helps in some way. Using version "U-Ambience 1.0-9-1.10.2-PR"

CircuitLord commented 7 years ago

Read the comments for this after I downloaded forge and setup a compiling workspace and compiled this branch. Great going me. At least I learned a bit about forge compiling lol.