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

Adding sound controller to the game #28

Closed
wants to merge 4 commits into from

Conversation

stelios357
Copy link

No description provided.

@stelios357 stelios357 mentioned this pull request Dec 19, 2020
2 tasks
@ParadoxZero ParadoxZero linked an issue Dec 20, 2020 that may be closed by this pull request
2 tasks
@ParadoxZero ParadoxZero changed the title ISDL13 Adding sound controller to the game Dec 20, 2020
Copy link
Owner

@ParadoxZero ParadoxZero left a comment

Choose a reason for hiding this comment

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

Hey folks,

Great work! I have put in some suggestions after quickly reviewing the PR.
Please make a note of removing the temporary code. I think I have found the reason for the error. I'll fix it when I get time.

}


void sound::onCollisionSound()
Copy link
Owner

Choose a reason for hiding this comment

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

Split this to two methods.

playDeathSound and playFoodSound

isFood = false;
}

void sound::BGM()
Copy link
Owner

Choose a reason for hiding this comment

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

startBGM might be a better name.

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

bgm.setLoop(true);
}

void sound::menuMusic(bool x)
Copy link
Owner

Choose a reason for hiding this comment

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

Try to start the method with a verb to better capture purpose of method.

bgm.setLoop(true);
}

void sound::menuMusic(bool x)
Copy link
Owner

Choose a reason for hiding this comment

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

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);

Choose a reason for hiding this comment

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

I divided the method into 2 parts i.e start and stop

Copy link
Owner

Choose a reason for hiding this comment

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

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




class sound
Copy link
Owner

Choose a reason for hiding this comment

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

SoundController or SoundManager will be the correct name. Since Sound hints to being an object wrapping over the .wav files.

Copy link

@dev-patel2104 dev-patel2104 Dec 20, 2020

Choose a reason for hiding this comment

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

I changed the class name to SoundController , will it be okay if the file name remained the same.

Copy link
Owner

Choose a reason for hiding this comment

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

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.


void game::MainMenu::start( sf::RenderWindow * w ) {
gmenu::Menu menu( w );
gmenu::Menu men( w ); // remeber to change this back to menu after completion
Copy link
Owner

Choose a reason for hiding this comment

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

I think you missed this. Also out of curiosity, why did you have to make this change?

Copy link

@dev-patel2104 dev-patel2104 Dec 20, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove the temporary fix.

@ParadoxZero
Copy link
Owner

#29 Should fix a lot of issues inherently.

@stelios357
Copy link
Author

I've made an another Pull Request for this.

@stelios357 stelios357 closed this Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need sound controller
3 participants