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

[CR] Add audio support for curses mode #78880

Merged
merged 5 commits into from
Jan 5, 2025

Conversation

JodiJodington
Copy link
Contributor

Summary

Features "audio now works on curses mode, not just tiles mode"

Purpose of change

Closes #78841

Describe the solution

this patch initializes audio support as long as you are building with SOUND=1 without requiring TILES=1

Describe alternatives you've considered

I don't see any problems with this solution except the few copy and pasted lines but I figured it would be a lot easier to read and write this way, if that's an issue I can try to "DRY" it

Testing

I compiled with make SOUND=1 TILES=0 and sound worked, tried the base sound pack and CC-Sounds and both worked, the api hasn't changed so I assume everything else also works as intended
I also did not check if it works on windows as I do not have a windows PC, but the code changes are small and mirrored in ncurses_def.cpp and wincurses.cpp so I can't imagine there would be any windows-specific bugs

Additional context

@github-actions github-actions bot added Code: Build Issues regarding different builds and build environments [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) <Enhancement / Feature> New features, or enhancements on existing astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Dec 31, 2024
@JodiJodington
Copy link
Contributor Author

I'm wondering what people think about this: should the COMPILING.md file be changed in this PR to say that SOUND=1 does not rely on TILES=1 anymore?
I was thinking of keeping this PR small and only doing what was necessary to get it to compile and run, but I don't know if that's the best way to do it

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 1, 2025
@Maleclypse
Copy link
Member

Hi Jodi,
Thank you for making this. I don't know anything about curses so can I ask you a couple questions?

My understanding of this and it's related issue is that this is just allowing curses players to bring over a soundpack and then play with sounds turned on?
Is there anything that needs to be added like a debug message as my understanding was that we don't ship sounds with the curses build because it's not enabled for it.
This PR does not cause sounds to be shipped with the curses build so if someone turned on sound in curses and there is no message telling them to go get a soundpack they may believe curses sound is broken?

@JodiJodington
Copy link
Contributor Author

JodiJodington commented Jan 4, 2025

as my understanding was that we don't ship sounds with the curses build because it's not enabled for it.

Ideally the default soundpack would ship with the curses build if built with sound support, do you know what I'd need to change for that to be the case? I changed the install target to install data/sounds even if not building with tiles support (see lines 1274-1277ish in the Makefile), would I need to do anything else?
Thank you for responding!

Makefile Outdated Show resolved Hide resolved
@harakka
Copy link
Member

harakka commented Jan 4, 2025

should the COMPILING.md file be changed in this PR to say that SOUND=1 does not rely on TILES=1 anymore?

Yes please.

@esotericist
Copy link
Contributor

to give a firmer response than harakka: yes, the documentation should be updated in the same PR to match the new behavior.

@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. [Markdown] Markdown issues and PRs labels Jan 4, 2025
@JodiJodington
Copy link
Contributor Author

It looks like apart from make and cmake, CDDA also supports the visual studio build system but I honestly can't figure it out and I don't have a windows PC so I don't know how to test it
to me it looks like SDL_SOUND is set unconditionally here but I don't really understand how the build system works

@JodiJodington
Copy link
Contributor Author

also I'm fairly sure I changed the only instance where the docs mention that tiles mode is required for audio support since nothing else that seems relevant comes up from a quick grep, but I just want to make sure no one else spots anything in the docs that I'd need to correct

@JodiJodington
Copy link
Contributor Author

the Windows CI test fail doesn’t seem to be related to my PR, what can I do about it?

@harakka
Copy link
Member

harakka commented Jan 4, 2025

It looks like apart from make and cmake, CDDA also supports the visual studio build system but I honestly can't figure it out and I don't have a windows PC so I don't know how to test it

We don't officially support terminal mode on Windows and make is the main build system, the rest are nice-to-haves that people who care about them maintain, so this should be fine.

the Windows CI test fail doesn’t seem to be related to my PR, what can I do about it?

I agree it doesn't seem related and the PR has BasicBuildPassed already, so it is mergeable even with that test failing.

@Maleclypse Maleclypse merged commit 0fdda05 into CleverRaven:master Jan 5, 2025
25 of 26 checks passed
@Maleclypse
Copy link
Member

Thank you for doing this!

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 <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments <Documentation> Design documents, internal info, guides and help. <Enhancement / Feature> New features, or enhancements on existing json-styled JSON lint passed, label assigned by github actions [Markdown] Markdown issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow audio support on ncurses builds
5 participants