Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Live Weather and Styling #10

Merged
merged 24 commits into from
Jan 31, 2016

Conversation

GammaGames
Copy link
Contributor

Added the option to get live weather using weather underground, from #5
Added alternate icons that change between play and pause to indicate #6 (not in options, just happens by default, could be added)
CSS to make the range, radio, and checkbox inpuits look nicer, renamed the css file to reflect this
town tune page has colors
Volume slider now goes in increments of 0.05 instead of 0.1

Stuff in ignore file:
Now ignores .mp3

Stuff in css file:
Modified width of pitch boxes so the text was more centered
Added CSS for range sliders (to color the actual slider thumb later)

Stuff in editor js file:
Now makes the pitch box be the note color
Modified default tune to match new notes
Changed flash color to white
Added function to get hex color from pitch

Stuff in player js file:
Changed available pitches
Fixed typo
Changes in extension.js:
Add option for automatic pausing
Add option for using current weather (using my api key, should add
option for custom key)
Icon image changes when pausing or unpausing
Change default tune to new format
Add function to check if tabs are audible (showing mic)
Add function to fade music in from silence
Add function to set current weather condtions

Changes in manifest.json:
Add openweathermap to permissions (hopefully it works)

Changes in options.html:
Fix radio buttons and checkboxes for css styling
Add beta section for WIP features
Add option for new features

Changes in tune_editor.css:
Add css for radio, checkboxes, text input, buttons, and default sliders
Fixed town tune editor colors
Changes in extension.js:
Added local weather variable for live weather (no longer synced in
options)
updateWeatherCond no longer requires url
If weather setting was just updated/extension just started set timeout
(updateWeatherCond is asynchronous)
Add option to show current weather as badge text
Move to new OOP approach
Add styling for input: radio, checkbox, text, range
Add option to show live weather using a zip and country code from
openweathermap's api
Add option to hide text on badge
Icon now uses pause/play pictures
Add pitch colors to Town Tune editor
Change available pitch to fit game editor more
Un comment out town tune setting
Update default town tune

Not sure what's causing the issue so the town tune isn't playing
anymore...
Town tune now correctly plays for hour changes
Add function to utilities, get correct audiosrc file depending on game
Fix text box background from gross yellow color
Updating to kylechu's latest commit
There are now 20 possible values for volume instead of 10.
Removed some stuff I had commented out that no longer worked
@kowsen
Copy link
Contributor

kowsen commented Jan 24, 2016

I'm not a git expert, but I think if you rebase this pull request based on the current state of the JdotCarver repo, it'll make the changes easier to see and should fix the conflicts.


addEventListener("hourMusic", playHourlyMusic);

addEventListener("weatherMusic", playWeatherMusic);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it would be better to handle determining what weather's music we want to play in StateManager rather than AudioManager. For example, if it is snowing and we're on the live weather option, just send "new-leaf-snowing" as the game in a normal hourMusic message. This setup clutters the system with two new events, and means if we want to change the way music plays, we would need to do it in multiple places - and I don't see any advantages to determining what music should be played in AudioManager that offset this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good point, and it would work a lot nicer if there were the WeatherManager object like you referenced later. To get the weather working I was modifying it like I had before, this is a much better solution

Fixed functions in badgeManager being in global namespace
Fixed AudioManager having control over the badge, now all contained in
BadgeManager
Moved several functions from Utility to a more appropriate location
# Conflicts:
#	css/tune_editor.css
#	manifest.json
#	options.html
#	scripts/AudioManager.js
#	scripts/BadgeManager.js
#	scripts/NotificationManager.js
#	scripts/StateManager.js
#	scripts/TownTuneManager.js
#	scripts/main.js
#	scripts/options.js
@GammaGames
Copy link
Contributor Author

Thanks for the tips kylechu! I've done some of them, you can see over at ec294bb.

Unfortunately I've got to work on classes right now, so I won't be able to work on the WeatherManager object for the next couple days at least, though I do agree it would go a long way with simplifying this.

@JdotCarver
Copy link
Contributor

Geez, so many additions! I have got to try them out! Possibly enough for the next release now.

@kowsen
Copy link
Contributor

kowsen commented Jan 25, 2016

I just remembered that I started trying to write a WeatherManager object when I first saw your code. I just fixed some bugs in it and I think got everything working - you can check it out here and take whatever ideas you want from it - e78a6d5.

@JdotCarver
Copy link
Contributor

Just tell me when you're ready for the merge and I'll implement it. :)

@JdotCarver
Copy link
Contributor

Btw Please reply under what name you'd wish to appear in the credits. :)

@mherndon6
Copy link

Hey, I've been working on a couple features on kylechu's branch. I added some styling to the options page and added the mix all option. I'll make it compatible with GammaGames's changes and submit another pull request tonight.

Borrowed ideas from e78a6d5 to fit the OOP format better
Fixed town tune sliders not updating color
# Conflicts:
#	manifest.json
#	scripts/AudioManager.js
#	scripts/StateManager.js
#	scripts/Utility.js
Merged with master again, new looping options should still work
@GammaGames
Copy link
Contributor Author

I merged twice on accident, the first automatic one added some <<<<<HEAD lines in places and I had to go through and check to make sure all the looping options were still working.
Might need to be modified a bit, I'm still using playPause(true) to star the audio

Should all be working now! I've updated adding a WeatherManager. The object checks for new weather every 10 minutes
For credit I'd like "Jesse Lieberg", thanks!

}

// get current weather conditions using openweathermap: http://openweathermap.org/current
function updateWeatherCond(zip, country, cb) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateWeatherCond is redundant and should be deleted if we have a weatherManager object

Removed old functions from AudioManager
Made KK icon show during his time
Re-add easier support for other games to do live music in StateManager
WeatherManager doesn't change on same category change
@GammaGames
Copy link
Contributor Author

Thanks for the tips again, you've been incredibly helpful in getting this clean and nice to work with :)
As far as I can see the features are working, but you should probably wait for kylechu's go ahead first since he obviously knows his stuff!

});

function play(kk) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not the biggest fan of the functions being called "play" and "pause", since that makes them sound like they're controlling the music. Maybe have one function called changeIcon that takes two variables - isPlaying and isKK?

This is just a minor nitpick though.

@kowsen
Copy link
Contributor

kowsen commented Jan 26, 2016

You're absolutely welcome, always happy to help and grateful for the confidence you've put in me! I'm happy with this pull request - everything's looking really nice! I haven't tested it thoroughly, but I will over the next few days, and it sounds like Gamma's done some tests.

The comment on the diff I made a few minutes ago is just a super minor nitpick. If Gamma agrees and wants to change it, it would make me happy, but I think it's good as-is.

Moves change badge icon to one function
Updated play/pause icons
Badge icon now has 100% opacity when playing, 50% when paused instead of
play/pause symbols
Included 50% opaque version for mherndon's branch
Changed icon files from "-01" and "-02" to "_paused" and "_playing"
@GammaGames
Copy link
Contributor Author

The play/pause badge icons always bugged me, so I took some inspiration from mherdon6's branch. I've only changed a few things in the latest commit:

  • I removed some extra icons that were never used (19 and 128 versions for most of them)
  • Changed the icons to the more New Leaf color, with 50% opacity when paused and 100% when playing.
  • Renamed the png files to make more sense

I've also included a 50% opacity icon of KK for mherdon6's alternate icon branch, because his work over there is pretty cool!
These changes should not have broken anything, I'm still running it regularly and haven't found any issues.

function getMusic() {
if(isLive()) {
if(weatherManager.getWeather() == "Rain")
return "new-leaf-raining";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only works in new leaf?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extension doesn't have any audio files for WW/CF raining, at least that I saw. It could always be added later though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get / upload them today.

JdotCarver added a commit that referenced this pull request Jan 31, 2016
@JdotCarver JdotCarver merged commit a7d853c into animal-crossing-music-extension:master Jan 31, 2016
@JdotCarver
Copy link
Contributor

Got the files in mp3 format, I'll reencode them in ogg with Audition when I come back to my main rig. (With the reduced bitrate and I'll update looping times with it too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants