Add web soundtrack #236

devmattrick · 2017-10-22T07:08:26Z

Adds a soundtrack to the webpage upon startup (also allows the ability to mute/ unmute and stores it in local storage), resolving both #230 and #231.

Please let me know if you'd like any changes made!

nirs · 2017-10-22T08:02:47Z

@devmattrick thanks for your contribution! Looks great so far, will review it later.

nirs

@devmattrick, great work!

I would like to merge the first commit with minor renaming. The next commits need little more work.

Can you remove the next commit from this PR and send a new PR for the second issue?

nirs · 2017-10-24T14:12:38Z

rose/web/game.js

@@ -17,16 +17,21 @@ var ROSE = (function() {
        this.controller = new Controller();
        this.rate = new Rate([0.5, 1.0, 2.0, 5.0, 10.0]);

-        var loader = new ImageLoader(function() {
+        var imageLoader = new ImageLoader(function() {


I would like to avoid mixedCase names in our code, they are more error prone and harder to read and write. Can rename this to image_loader?

nirs

The third commit should be squashed into the seconds, and it would be nice to move both to a new PR.

nirs · 2017-10-24T14:16:04Z

rose/web/game.css

@@ -41,6 +41,17 @@ body {
    margin: 0;
 }

+#toggle_mute {


Maybe sound_ctl?

nirs · 2017-10-24T14:18:28Z

rose/web/game.css

+    padding: 8px 8px;
+    position: absolute;
+    left: 50%;
+    transform: translateX(-50%);


This puts the sound button in a rather central location. I think we need to align this to the right, before or after the game rate control.

nirs · 2017-10-24T14:24:44Z

rose/web/game.js


        this.context = $("#game").get(0).getContext("2d");
        this.dashboard = new Dashboard(imageLoader);
        this.track = new Track(imageLoader);
        this.obstacles = new Obstacles(imageLoader);
        this.cars = new Cars(imageLoader);
        this.finish_line = new FinishLine(imageLoader);
+        this.sound_controller = new SoundController(soundLoader);


Having a sound_controller in the application is nice idea. But this object has no behavior currently.

We are starting with adding the current soundtrack, and having a way to mute it, but I think in the long term we would like to have different sounds for different game states:

No players connected - maybe no sound

Player connected, game not started - revving engines?

Game running - racing sounds

Game ended - applause

So I expect that this sound controller will have some behavior like toggle_sound(), and bind this api to the button. Then the same api can be used also from application code if needed.

nirs · 2017-10-24T14:28:12Z

rose/web/game.js

+        this.loader = loader;
+
+        var localStorageMute = localStorage.getItem('muted');
+        this.muted = (typeof localStorageMute !== 'undefined') ? localStorageMute == "true" : false;


It would be nice to move the code loading and saving the config to a method of this object.

Also, is "muted" specific enough? maybe we like to save the config under "rose.sound.muted"?

nirs · 2017-10-24T14:32:59Z

rose/web/game.js

+
+        var self = this;
+
+        $("#toggle_mute img").attr('src', icons[this.muted]);


I think there is a bug here, after muting the sound and reloading the page, I see the wrong button. But maybe I'm just confused by seeing the unmute button when I expect the mute button.

Maybe we need to have sound-on.svg and sound-off.svg instead of mute and unmute. When the sound is muted, we show the sound-off icon, and when the sound is not muted, the sound-on icon.

In this case, when the sound is muted, I expect to see a grayish button, meaning "sound is off". When sound is not muted, I expect the see more lighter button showing "sound is on".

nirs · 2017-10-24T15:45:57Z

rose/web/game.js

+
+        loader.load("res/soundtrack/Nyan_Cat.ogg", function(sound) {
+            sound.muted = self.muted;
+            sound.loop = true;


Don't we need this in the previous commit? The sound should always loop, regardless of having a way to mute it.

nirs · 2017-10-24T15:47:27Z

rose/web/game.js

+            event.preventDefault();
+
+            self.muted = !self.muted;
+            $(this).children('img').attr('src', icons[self.muted]);


I think we should use:

$("#toggle_sound img").attr(...)

nirs · 2017-10-24T15:51:38Z

rose/web/index.html

@@ -14,6 +14,8 @@
        <button id="start" disabled>Start</button>
        <button id="stop"  disabled>Stop</button>

+        <button id="toggle_mute"><img src="res/icons/mute.svg"></button>


This possibly starts with the wrong icon, I wonder if we should have a third icon for the loading step, or make this button disabled until we read the state from local storage.

devmattrick · 2017-10-25T02:29:10Z

Thanks for the suggestions, I'll work on it as soon as I get a chance!

This reverts commit dd9e694.

nirs · 2017-10-26T16:03:34Z

rose/web/game.js

+        });
+    }
+
+    SoundController.prototype.toggle_sound = function(sound_id) {


I'm not sure how you are going to use this in the next version, since when clicking sound button, we have to:

play/pause sound

replace the icon

It seems that the entire sound controller belongs to the next PR, this PR is happy with the plain loader, playing the file when it was loaded, as in the first commit.

This reverts commit 8d877e2.

nirs · 2017-10-26T21:35:22Z

@devmattrick thanks for this fine work! I hope you can find time to send the next PR for #231.

Adds a soundtrack to the webpage upon startup. Fixes RedHat-Israel#230.

mattrick added 3 commits October 21, 2017 23:25

Add soundtrack to webpage start

da59d62

Add ability to mute and unmute sound

dd9e694

Make img selector for mute button more specific

a6bc5c4

devmattrick changed the title ~~Web sound~~ Add web soundtrack Oct 22, 2017

nirs added the hacktoberfest label Oct 22, 2017

nirs requested changes Oct 24, 2017

View reviewed changes

mattrick added 2 commits October 26, 2017 00:13

Revert "Add ability to mute and unmute sound"

8966e5d

This reverts commit dd9e694.

Use more reusable sound controller

8d877e2

nirs reviewed Oct 26, 2017

View reviewed changes

mattrick added 2 commits October 26, 2017 13:56

Revert "Use more reusable sound controller"

7592e53

This reverts commit 8d877e2.

Switch to snake_case

ffd7ba3

nirs approved these changes Oct 26, 2017

View reviewed changes

nirs merged commit c89e05b into RedHat-Israel:master Oct 26, 2017

rodxavier pushed a commit to rodxavier/ROSE that referenced this pull request Oct 28, 2017

Add web soundtrack (RedHat-Israel#236)

07a32ab

Adds a soundtrack to the webpage upon startup. Fixes RedHat-Israel#230.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add web soundtrack #236

Add web soundtrack #236

devmattrick commented Oct 22, 2017 •

edited

Loading

nirs commented Oct 22, 2017

nirs left a comment

nirs Oct 24, 2017

nirs left a comment

nirs Oct 24, 2017

nirs Oct 24, 2017

nirs Oct 24, 2017

nirs Oct 24, 2017

nirs Oct 24, 2017

nirs Oct 24, 2017

nirs Oct 24, 2017

nirs Oct 24, 2017

devmattrick commented Oct 25, 2017 •

edited

Loading

nirs Oct 26, 2017

nirs commented Oct 26, 2017


		var self = this;

		$("#toggle_mute img").attr('src', icons[this.muted]);

Add web soundtrack #236

Add web soundtrack #236

Conversation

devmattrick commented Oct 22, 2017 • edited Loading

nirs commented Oct 22, 2017

nirs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devmattrick commented Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

nirs commented Oct 26, 2017

devmattrick commented Oct 22, 2017 •

edited

Loading

devmattrick commented Oct 25, 2017 •

edited

Loading