animal-crossing-music-extension / ac-music-extension

Google Chrome extension that plays hourly Animal Crossing music and more while browsing!
https://acmusicext.com
zlib License
344 stars 57 forks source link

New Horizons hourly bell chime + speed adjustment #130

Closed Xane123 closed 4 years ago

Xane123 commented 4 years ago

This extension is nearly perfect to me, but I've noticed the hourly bell chime isn't close to the actual games' bells. I viewed the history of bells.ogg and saw New Leaf's bells were briefly added, then reverted, but I think I've improved upon that.

This pull request replaces the current bells with ones ripped from New Horizons, which reverb and last 4 seconds each, the exact same length that the current (and NL experiment)'s chimes are! In addition, the chime itself was too fast, so I slowed it down, nearly matching the town theme's speed in NH.

Here is how my town tune sounds with this commit: New chime demo.zip

Xane123 commented 4 years ago

Yep, I think I got the timing very accurate (changing the dividing number from 60 to 90). Before I made this pull request, I compared my timing to the town tune in the game and they were nearly perfectly in sync. image There's technically one thing left that's wrong about the chime. In the games, there's about a second of silence after the last chime before the next hourly music plays, but this extension only waits for the final note's length then plays immediately as the bell is fading out. I was lucky with these small changes, but do not know how I could make the chime delay the music like that.

Julian-MJK commented 4 years ago

Really nicely done, I think these new bells sound good!

.

However it also seems to have changed the Town Tune Editor, so that it plays each note with the same instrument, but for much shorter, so each bell ends too quickly and it ends up sounding stuttery and not like it does in the games.

For information, the town tune uses two different "instruments":

All these methods are in /scripts/global/tune_player.js.

I'd say the way the town tune sound when it's played at the hour is far more important, but we want to keep the town tune editor sounding the way it does in the games, if possible.

.

And to add a second delay to the town tune, you would have to add it to line 222 of /scripts/global/tune_player.js, so instead of if (callback) setTimeout(callback, stepDuration * tune.length * 1000); you would have if (callback) setTimeout(callback, stepDuration * tune.length * 1000 + 1000); where the last "1000" is the milliseconds of delay after the town tune is over, before continuing to play the music.

Xane123 commented 4 years ago

Oh, it does seem my changes slowed down the town tune editor, you're right. I don't know much about JavaScript, so I've hit a wall here. Basically, I was looking at the getStepDuration function where I slowed down the chime by dividing the value by 90 instead of 60, and thought about using a conditional variable set (the "condition ? true : false" thing).

I added "instrument" to the function's argument list and passed the instrument to it. I expected it to return 'booper' or 'sampler', so I could dynamically divide by 90 if it's the chime or 60 in the editor, but it seems the "instrument" argument is an object ("[object Object]") and not a simple string, so I have no clue how I could check for which instrument that it's using.

stepDuration = (instrument == 'sampler') ? 1 / (bpm / 90) : 1 / (bpm / 60); always returns the "false" value and now plays both at the faster speed, which isn't correct for the hourly chime. With my limited knowledge, the only workaround I could think of is to add another argument to the "PlayTune" function that is set to 0 or 1 based on if it's the sampler, but I didn't do it because I feel it would add unnecessary, redundant complication to the code. If it's okay for me to do that, I will do that, which should fix the town tune editor playing at the wrong speed.

Julian-MJK commented 4 years ago

The reason it always returns false is because it's an object, which cannot be compared to a string, so it just returns false all the time.

That could be solved by adding a simple string variable to the object, so that when you need to find out what the instrument is, you could just do if(instrument.instrumentName == "booper") { do_thing() }

For example by adding var instrumentName = "booper"; to line 17 of tuneplayer.js (just under "var createBooper = function(audioContext){")_

And by adding var instrumentName = "sampler"; to line 92 of tuneplayer.js (under "var createSampler = function(audioContext){")_

So instead of stepDuration = (instrument == 'sampler') ? 1 / (bpm / 90) : 1 / (bpm / 60); you would write stepDuration = (instrument.instrumentName == 'sampler') ? 1 / (bpm / 90) : 1 / (bpm / 60); and it would return the name of the instrument being used. This simple addition could come in handy in many different places really, without over-complicating things, so hell yeah go ahead ^^

Xane123 commented 4 years ago

Alright, I've made the requested changes (which really are genius changes once I thought about how it all works internally). At 6PM, I'll hear if the bell chime has returned to its intended slower speed, and if so, I suppose it could be considered ready for merge. I will also be able to test if the pause after the bell chime is good enough. (I set it to "stepDuration * 2", which should be the equivalent of two notes in the town tune.)

Alright, strangely even with these changes, the bell is still playing at the faster speed. I've updated the unpacked extension in Chrome, and it is pointing to my Git repository, so it must be using the updated scripts. When I use an "alert" dialog box to display what instrument.instrumentName's value is when the getStepDuration function is executing, Chrome returns it's "undefined". My previous commit shows that I added the variable in both of the functions, though. image

Julian-MJK commented 4 years ago

I tested the branch out, and it now seems to work perfectly!

The town tune in the editor sounds just like it does in the master branch, and the town tune at the hour sounds faster but still good.

I'm a little unsure on how fast the town tune should be at the hour however, I can't remember it nor find any reference on YouTube (and I haven't unlocked the AC:NH town tune yet), so it may need tweaking, but I'm not sure. Could you provide a in-game reference video, or could anyone else confirm that it sounds well?

Other than that, all the code additions look good, the new parameter you added to getStepDuration has been accounted for in every line of code that uses the method, which is exactly one line, and the branch works as expected. I would like someone else's opinion on this before approving however, how it sounds, @PikaDude @paladique @JordiGarcL @ilikecorndogs could one of you check if it sounds right?

Xane123 commented 4 years ago

I believe I've found what might do it. In TownTuneManager.js, it calls "playTune" with a BPM of 100, compared to the editor's 240. I'm going to see if dropping this leads to a possibly closer speed, which I'll compare with the game's speed soon. I think this is the method I'll use instead of altering the step duration. It'll be nice if someone could find out why the script wasn't getting the intended string values from instrumentName, just for if anyone down the line wants to check for either instrument in scripts.

I'll fine-tune this value and try to get it close to the timing it had with the "divide by 90" step duration and I'll update this with another comment once I have a comparison to the game. If I can get this right, it will be ready for review.

So far, setting the BPM to 67 (from 100) makes it pretty close to the older timing so I think it's probably pretty close to the game.

Xane123 commented 4 years ago

Okay, I think I've found the closest BPM for the hourly bell chime, 66! When it's set to play at that tempo rather than 100, it plays at a speed comparable to New Horizons, but just slightly slower. If I make it 67, it goes too fast, so this is the best speed.

Here's a ZIP file containing a recording of my island tune, both in-game and with my extension changes. ChimeComparison.zip

paladique commented 4 years ago

Looks and sounds good! Thanks for your contribution, @Xane123!