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

Expand playlist ids for soundpack #56045

Merged
merged 19 commits into from
Mar 16, 2022
Merged

Conversation

MegasKomnenos
Copy link
Contributor

@MegasKomnenos MegasKomnenos commented Mar 12, 2022

Summary

Features "Add new id hooks that soundpack creators can use to control which playlist is played at which situation."

Purpose of change

Soundpack creators can use the 'playlist' feature to add a playlist of background musics. However, there is practically no control on which playlist will be played at a given situation. There's is only one id that playlists can use, which is 'title', and that's really not that much. Specifically, I wanted to give players an experience similar to fallout, where you can turn on the radio and listen to the music being played.

Describe the solution

I added a set of new functions in music.h/music.cpp which manages which music id is activated at a given time. I added function call to its related functions in iuse.cpp, iuse_actor.cpp sounds.cpp, sdlsound.cpp, do_turn.cpp, and main_menu.cpp to make correct music id be activated at a correct situation, and to make correct playlist be played depending on currently activated music ids.

Testing

More help would be very welcome. I did some very rudimentary tests where I opened up a game and spawned in acoustic guitar, mp3 player, smartphone, and stereo to confirm that they work. My machine is in linux, and I did not test in windows.

Additional context

I made a test soundpack to use for testing this feature.
https://drive.google.com/file/d/1w0fhbKPmPTxOg_152Qw__Bawhq2xATIf/view?usp=sharing

stop_music function did not clear the playlist_indexes vector.
Update master to original project
# Conflicts:
#	src/do_turn.cpp
It now supports 4 different ids of,

1. mp3
2. instrument
3. sound
4. title

mp3 is triggered when an item with MP3 flag is activated.
instrument is triggered when a musical instrument is played by the player.
sound is triggered when player character hears any sound that is classified as music
title is the base. It will always remain triggered.

When multiple ids are triggered at the same time, the one with higher priority(the order is as written above) gets played.
@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions labels Mar 12, 2022
@NetSysFire NetSysFire added <Enhancement / Feature> New features, or enhancements on existing SDL: Tiles / Sound Tiles visual interface and sounds. labels Mar 12, 2022
@NetSysFire
Copy link
Member

You need to fix your PR summary but you almost got it right. See https://github.com/CleverRaven/Cataclysm-DDA/blob/master/.github/CONTRIBUTING.md#all-prs-should-have-a-summary-section-with-one-line

Just remove the dummy 'Category "Brief summary"'

@NetSysFire
Copy link
Member

Also ping @dseguin, looks like you have done lots related to the sound system in the past so you may be interested in this PR

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 12, 2022
@github-actions github-actions bot removed the SDL: Tiles / Sound Tiles visual interface and sounds. label Mar 12, 2022
@MegasKomnenos
Copy link
Contributor Author

You need to fix your PR summary but you almost got it right. See https://github.com/CleverRaven/Cataclysm-DDA/blob/master/.github/CONTRIBUTING.md#all-prs-should-have-a-summary-section-with-one-line

Just remove the dummy 'Category "Brief summary"'

Thanks! I think I changed it correctly based on your suggestion, though I'm very new to all this so it's entirely possible I still messed up somehow lol.

Copy link
Member

@anothersimulacrum anothersimulacrum left a comment

Choose a reason for hiding this comment

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

Changes are not required (except maybe newlines at EOF), just make this a little more in cata's style.

src/music.cpp Outdated Show resolved Hide resolved
src/music.cpp Outdated Show resolved Hide resolved
src/music.cpp Outdated Show resolved Hide resolved
src/music.cpp Outdated Show resolved Hide resolved
src/music.cpp Show resolved Hide resolved
src/music.cpp Show resolved Hide resolved
src/music.cpp Outdated Show resolved Hide resolved
src/music.h Outdated Show resolved Hide resolved
Copy link
Member

@dseguin dseguin left a comment

Choose a reason for hiding this comment

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

I agree with anothersimulacrum's suggestion, it might be better to add the enum conversions as a io:: template (see widget.cpp, line 56 for some examples).

I think it would be a bonus if the new headers could be added to the list in alphabetical order (ex: #include "music.h" between #include "mtype.h" and #include "mutation.h")

(again, changes are not necessary, except for the string comparisons because it triggers clang-tidy)

Otherwise, looks good! Glad to see people working on the sound stuff :)

src/do_turn.cpp Outdated Show resolved Hide resolved
src/sdlsound.cpp Outdated Show resolved Hide resolved
src/music.cpp Outdated Show resolved Hide resolved
Mainly,

1. Change music_id_list to use std::map, and change related codes accordingly.
2. Reimplement enum_to_string for music_id in io namespace, and remove my separate implementation of string_to_enum
3. Place #include directive for music.h in correct alphabetical order alongside its buddies.
4. Add whitespace before EOF as per project convention.
5. Replace string comparison method call with comparison operator.
@MegasKomnenos
Copy link
Contributor Author

Changes are not required (except maybe newlines at EOF), just make this a little more in cata's style.

I agree with anothersimulacrum's suggestion, it might be better to add the enum conversions as a io:: template (see widget.cpp, line 56 for some examples).

I think it would be a bonus if the new headers could be added to the list in alphabetical order (ex: #include "music.h" between #include "mtype.h" and #include "mutation.h")

(again, changes are not necessary, except for the string comparisons because it triggers clang-tidy)

Otherwise, looks good! Glad to see people working on the sound stuff :)

Thanks for reviewing my code! I applied most of the suggested changes in this commit(MegasKomnenos@fc934d8). It's nice getting a better grasp of the conventions and stuff, so thanks for helping me out with that!

And yeah, sound is a really cool part of any game, and I think I wanted to expand upon that based on an idea that I got.

@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Mar 12, 2022
@MegasKomnenos
Copy link
Contributor Author

So, are there more stuffs remaining that I can do for the merge?

@ZhilkinSerg ZhilkinSerg merged commit 7a8b2ef into CleverRaven:master Mar 16, 2022
@Fris0uman
Copy link
Contributor

Can you update the documentation with this neaw feature please? https://github.com/CleverRaven/Cataclysm-DDA/blob/master/doc/SOUNDPACKS.md

@MegasKomnenos
Copy link
Contributor Author

Can you update the documentation with this neaw feature please? https://github.com/CleverRaven/Cataclysm-DDA/blob/master/doc/SOUNDPACKS.md

Sure. I was about to make a new PR that fixes some issues, so I think I can do that along with what I was going to do anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` <Enhancement / Feature> New features, or enhancements on existing json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants