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

First Audio Implementation #33

Merged
merged 9 commits into from Oct 10, 2019

Conversation

@capnkenny
Copy link
Member

commented Sep 4, 2019

#13, AB#14, AB#15

Initial Audio Implementation:
-Supports WAV, OGG, MP3
-Implements AudioService to manage all channels, effects, as well as loading/start/stop/pause of music (BGM) and sounds (SFX)

@capnkenny capnkenny self-assigned this Sep 4, 2019
@capnkenny capnkenny added the engine API label Sep 4, 2019
@capnkenny capnkenny force-pushed the feature/audio_v3 branch from adb17af to 8c66313 Sep 13, 2019
Copy link
Member

left a comment

Mostly just minor things. Code cleanup etc. Though some C-like things should probably be done C++-like instead.

src/NovelAudioChannel.cpp Outdated Show resolved Hide resolved
src/NovelAudioChannel.cpp Outdated Show resolved Hide resolved
src/NovelAudioChannel.cpp Outdated Show resolved Hide resolved
src/NovelAudioChannel.cpp Outdated Show resolved Hide resolved
src/NovelAudioChannel.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
@CLAassistant

This comment has been minimized.

Copy link

commented Oct 5, 2019

CLA assistant check
All committers have signed the CLA.

@capnkenny capnkenny requested review from RubyNova and tannergooding Oct 6, 2019
@capnkenny capnkenny marked this pull request as ready for review Oct 6, 2019
Copy link
Member Author

left a comment

This review should have been resolved a while ago, oops 😅
EDIT: In reference to Ruby's first review

Copy link
Member

left a comment

Functionally looks good (I've yet to test it on Ubuntu but should be fine) but the code is kind of hard to read in places due to magic numbers. Please refactor/advise.

src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Show resolved Hide resolved
src/NovelAudioService.cpp Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
src/NovelAudioService.cpp Outdated Show resolved Hide resolved
@RubyNova

This comment has been minimized.

Copy link
Member

commented Oct 6, 2019

As an additional note - you should update the readme as it says audio is coming soon, but with this branch it'll actually exist. 😄

src/main.cpp Outdated Show resolved Hide resolved
@tannergooding

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

Please fix the commit history (happy to help if needed).

This is reintroducing practically every commit from master (it looks like a bad rebase).

@capnkenny

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2019

Please fix the commit history (happy to help if needed).

This is reintroducing practically every commit from master (it looks like a bad rebase).

plz halp ;_;
Once I am active again, I'll ping in discord?

Used diff patch method to clean up history.
@capnkenny capnkenny force-pushed the feature/audio_v3 branch from a3a776f to 8c07f30 Oct 8, 2019
capnkenny added 3 commits Oct 8, 2019
Fixed instances where magic numbers did not correctly align with use cases.

Condensed loading and playing functions for Music and Sounds.

-9 is still gone.

UpdatedREADME
Fixed copyright symbol (from history commit - did not patch properly).

Removed unecessary function.
@capnkenny capnkenny requested a review from tannergooding Oct 8, 2019
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

left a comment

Overall looks reasonable.

I'd like to see bugs logged for the remaining comments and potentially some minor cleanup of the sample code.

As per Tanner's suggestions, renamed vars to audio button and added text.
Hadn't actually fixed references to the constants that were moved x_x oops.
@capnkenny

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2019

Addressed the bugs / nits / cleanup from Tanner's second review, and created issues for outstanding topics. Leaving for Matt to dismiss his requested change and to merge in the AM 👍

@tannergooding

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

Found two things:

  1. Could you modify scripts\build.ps1 and scripts\build.sh. In both, just change install freetype glad glm lua sdl2 sdl2-image to include sdl2-mixer at the end
  2. The sample doesn't work out of the box because the wav files aren't checked in, but they are binary files so we probably don't want to check them in. @RubyNova ?
@capnkenny

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2019

Good catch, I'll make those changes in a bit. And yeah, I didn't want to check in large binary files (oof .wav filesizes) so I uploaded them to GDrive and posted it in Discord. Let's see what @RubyNova wants to do about that.

capnkenny added 2 commits Oct 8, 2019
@RubyNova RubyNova merged commit 48beaff into master Oct 10, 2019
1 check passed
1 check passed
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.