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

Revamp the sound engine. #236

Merged
merged 13 commits into from
Mar 25, 2019
Merged

Revamp the sound engine. #236

merged 13 commits into from
Mar 25, 2019

Conversation

TheRamenChef
Copy link
Collaborator

The main feature of this pull request is a total rewrite of the SoundPlayback implementation. Notable changes include:

  • A resource leak in the previous implementation has been fixed (every time a sound looped, it would open a new line without closing the previous one).
  • There is now a Track interface for more complex music, such as music with an intro.
  • There is no longer an audible seam between looping sounds, as the data line is reused.

Also included is an implementation of layer opacity that I had previously forgotten to write.


This is currently marked as a draft pull request as there is a major bug in my implementation that makes some sounds not play. It's extremely annoying, as it's highly intermittent and I've been unable to determine the cause. It is assigned a thread, but for some reason the task exits before it can write any data to the line, but it's not being cancelled, because if it was, it would hit a breakpoint. You have implemented a sound playback before; what am I doing wrong?

@steffen-wilke
Copy link
Contributor

Just as a first note: The SoundEngine is designed to be accessed via Game.audio(). Regardless of the functional changes in this PR, this will stay the same. Not just for the SoundEngine but also for all the other major components of the engine.

@TheRamenChef
Copy link
Collaborator Author

I've never really asked this before, but why are all these components singletons, instead of static classes?

@steffen-wilke
Copy link
Contributor

It's a design decision we've made. The main benefit is that the Game class exposes all the API of the engine. This way, the developer doesn't have to remember the name of all the static classes but instead can just type in Game. and intellisense will figure out all the options.

@steffen-wilke
Copy link
Contributor

I've cherry-picked the general commits fc6b586 and cb663c1 and will now proceed with getting the actual SoundEngine changes into the master branch

@TheRamenChef
Copy link
Collaborator Author

Keep in mind that this implementation has a major bug in it which is why this is only a draft pull request. I assigned you in the hopes that you would be able to fix it.

@steffen-wilke
Copy link
Contributor

@TheRamenChef Thank's for the hint, I've just started to play around with the implementation and it also seems like the sound volume is not applied to the sound playbacks anymore. I'm currently not sure if this should be merged to the master before 0.4.16. I think this needs some more time for investigation and to mature.

@TheRamenChef
Copy link
Collaborator Author

The volume isn't dB gain; it's an actual multiplier on the amplitude of the sound wave. You should get a noticeable response from setting it to 0.1 or such.

The problem appeared to be with the Future's implementation of the
cancel method. Whatever it was, it seems to be gone now.
@TheRamenChef TheRamenChef marked this pull request as ready for review March 23, 2019 02:33
@TheRamenChef
Copy link
Collaborator Author

I've managed to fix the issue, whatever it was. This pull request is now live.

If you want to undo the sound engine being made static, go ahead.

@steffen-wilke
Copy link
Contributor

Perfect, will do so this weekend.

@steffen-wilke
Copy link
Contributor

The volume isn't dB gain; it's an actual multiplier on the amplitude of the sound wave. You should get a noticeable response from setting it to 0.1 or such.

I just set it to a minimum in the settings but this doesn't have any influence and the sounds are still played at normal volume.

image

The music volume is evaluated properly it seems.

@TheRamenChef
Copy link
Collaborator Author

Oh, you were talking about the sound volume in the settings. I just forgot to implement that. I've done that now.

Key components of the engine are accessed via the Game class in a Singleton manner instead of static classes.
Make sure that PhysicsEngine, RenderEngine and SoundEngine can only be initialized by the Game class.
@TheRamenChef
Copy link
Collaborator Author

It's working now; the Travis CI build seems to have the same problem as my early pull requests.

@steffen-wilke steffen-wilke merged commit d5c9d28 into master Mar 25, 2019
@steffen-wilke steffen-wilke deleted the newsound branch March 25, 2019 23:17
@steffen-wilke
Copy link
Contributor

It's working now; the Travis CI build seems to have the same problem as my early pull requests.

I think the breaking CI on the branch was due to some changes I recently made to the Travis configuration that were not merged to the branch. So the actual build succeeded but the SonarQube part failed.

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.

2 participants