Adding sound controller to the game #28
Conversation
src/Game-core/sound.cpp
Outdated
| } | ||
|
|
||
|
|
||
| void sound::onCollisionSound() |
There was a problem hiding this comment.
Split this to two methods.
playDeathSound and playFoodSound
src/Game-core/sound.cpp
Outdated
| isFood = false; | ||
| } | ||
|
|
||
| void sound::BGM() |
There was a problem hiding this comment.
startBGM might be a better name.
There was a problem hiding this comment.
Here i simply used BGM because this is doing both the tasks of starting and stopping the background music. And it is working in way that at every alternate calls it will start and stop the BGM
There was a problem hiding this comment.
This makes the actual function of method hard to understand when reading the code unless you open the function definition. Consider splitting it into StartBGM and StopBGM
src/Game-core/sound.cpp
Outdated
| bgm.setLoop(true); | ||
| } | ||
|
|
||
| void sound::menuMusic(bool x) |
There was a problem hiding this comment.
Try to start the method with a verb to better capture purpose of method.
src/Game-core/sound.cpp
Outdated
| bgm.setLoop(true); | ||
| } | ||
|
|
||
| void sound::menuMusic(bool x) |
There was a problem hiding this comment.
Better design is to split this into Start and Stop methods. Or alternatively change the signature to something like this-
enum MusicStatus
{
Start,
Stop
}
setMenuMusic(MusicStatus status);
There was a problem hiding this comment.
I divided the method into 2 parts i.e start and stop
There was a problem hiding this comment.
Same thing as before passing the bool, What is it doing?
Else rename the method as SetMenuMusic. That way it becomes
setMenuMusic(true) which anyone reading the code will know that it's going to loop the menu music as opposed to menuMusic(true) which can be anything like should the music loop? use 5.1 channel? etc
src/Game-core/sound.h
Outdated
|
|
||
|
|
||
|
|
||
| class sound |
There was a problem hiding this comment.
SoundController or SoundManager will be the correct name. Since Sound hints to being an object wrapping over the .wav files.
There was a problem hiding this comment.
I changed the class name to SoundController , will it be okay if the file name remained the same.
There was a problem hiding this comment.
Ideally file name should reflect the class name. Since in this case only 1 class exists in the file I would strongly suggest renaming the file as well.
src/UI/MainMenu.cpp
Outdated
|
|
||
| void game::MainMenu::start( sf::RenderWindow * w ) { | ||
| gmenu::Menu menu( w ); | ||
| gmenu::Menu men( w ); // remeber to change this back to menu after completion |
There was a problem hiding this comment.
I think you missed this. Also out of curiosity, why did you have to make this change?
There was a problem hiding this comment.
Oh sorry actually in order to fix the error temporary i used menu[] array so i just changed the name there so it wouldn't give me error. You can see menu array is declared at the bottom of MainMenu class in MainMenu.h under the private part.
There was a problem hiding this comment.
If you don't require anymore. Please remove these workaround code.
src/main.cpp
Outdated
| menu.start(&window); | ||
| so.menuMusic(true); // plays menu music | ||
| // This entire loop is just for the temporary fix for the create menu error |
There was a problem hiding this comment.
Please remove the temporary fix.
|
#29 Should fix a lot of issues inherently. |
|
I've made an another Pull Request for this. |
No description provided.