[WIP] Audio 2.0 #441

Kjarrigan · 2018-03-13T15:18:33Z

As the Sample-Song-Merge is quite a big thing (at least in my opinion) I already start this PR even if there is not much to show at the moment BUT I think its already worth discussing.

So here we are. I basically only changed Audio.hpp and test_audio.rb so far to show how I think the final interface would/should look like after merge.

Kjarrigan · 2018-03-13T15:19:45Z

Gosu/Audio.hpp

+    {
+        struct SoundData;
+        std::shared_ptr<SoundData> data;
+        std::vector<Channel> current_songs;


Not sure about the naming but thats basically the global variable you mentioned in #117

streaming_sounds? This doesn't have to be part of Audio.hpp, it can be a static variable in Audio.cpp.

Kjarrigan · 2018-03-13T15:20:36Z

Gosu/Audio.hpp


        //! Constructs a sample that can be played on the specified audio
        //! system and loads the sample from a file.
-        explicit Sample(const std::string& filename);
+        explicit Sound(const std::string& filename, bool streaming = false);


Are there named parameters in c++. Googling resulted in different opionions on how to and if at all...

Not really. You can build them yourself but I try to use only basic features in Gosu (both C++ and Ruby).

Kjarrigan · 2018-03-13T15:21:08Z

Gosu/Audio.hpp

-
-    //! Songs are less flexible than samples. Only Song can be played at any given time,
-    //! and there is no way to control its pan (stereo position) or speed.
-    class Song


Should Song be available in C++ for backwardscompat as well?

Hmmm...I would say yes. At least for now :)

Kjarrigan · 2018-03-13T15:25:31Z

Gosu/Audio.hpp

-        //! \param volume Can be anything from 0.0 (silence) to 1.0 (full
-        //! volume).
-        void set_volume(double volume);
-
        //! Called every tick by Window for management purposes.
        static void update();


Is there a good reason why this can't be available in Ruby as well? It would at least to some degree solve issue #390 and has no negative side effects as far as I can tell. This way we can get rid of the window in the tests and the guy who used gosu just to play music needs some kind of mainloop one way or the other. I'm not sure if playing sounds without window should work out of the box in Gosu, as its a "game development" lib 😄

Good point. Maybe there's some way in which Gosu could spawn a thread that keeps all sounds running, but in the meantime, Ruby users should have access to this.

Kjarrigan · 2018-03-13T15:28:03Z

lib/gosu/compat.rb

-  alias initialize_without_window initialize
-
+# No need to pass a Window to Sample and even better don't use Sample at all.
+class Gosu::Sample < Gosu::Sound


Inheritance because we got the double deprecation, but maybe just replace the whole construct with deprecate_const?

+1 deprecate_const, Gosu::Sound's interface should be compatible with Sample's

Kjarrigan · 2018-03-13T15:29:13Z

lib/gosu/compat.rb

  end
 end

-# No need to pass a Window to Song.
+# No need to pass a Window to Song and even better don't use Song at all.
 class Gosu::Song


Depending on whether or not the c++ counterpart of Song should still exists this might need inherit from Gosu::Sound too.

Kjarrigan · 2018-03-13T15:29:39Z

lib/gosu/compat.rb

@@ -165,7 +167,6 @@ module Gosu
  module Button; end

  # Channel was called SampleInstance before Gosu 0.13.0.
-  SampleInstance = Channel


Thats not necessary

Looks like that's right, I was worried about people writing if x.is_a? Gosu::SampleInstance.

This still works and triggers the deprecation warning too.

Kjarrigan · 2018-03-13T15:30:14Z

test/test_audio.rb

-    sound = Gosu::Sample.new(media_path("0614.ogg"))
-    channel = sound.play(1, 1, true)
+    sound = Gosu::Sound.new(media_path("0614.ogg"))
+    channel = sound.play(volume: 1, speed: 1, looping: true)


I thought, that if we already "drastically" change the naming part we might as well make this a hashed list as well?

I know that Ruby 2.1+ supports some form of named arguments on the language level, and I've been meaning to look into this, but I haven't found the time.

I like the proposed interface, but I would still provide the old interface for compatibility (so that we can just Sample = Sound for now), maybe with a deprecation warning. The C++ interface needs to stay the same, anyway...

Also, this would be a good way to deprecate play_pan. I think the issue is that play used to be louder than play_pan(0) at some point in time; I don't think that's true anymore, and even if it is, then the default value for pan could just be nil instead of 0 to distinguish both cases.

I thought about doing it the same way you handled Image.from_text. There you already deprecated a argument list and hinted towards the option-hash. if we internally use a Hash (to be backward compat to Ruby <2.0) or named parameters is just a matter of personal preference (and maybe a bit about more or less code). Ruby 2.1 only introduced require-named-parameters which also reduces a bit of code by doing some checks and raising an error automatically.
So I guess you're the one who defines whats the lowest Ruby Version Gosu should support?

I was planning to raise the minimum version to 2.3 right after Gosu 1.0. Some Gosu-related gems are still only compatible with Ruby 1.9.3(!) as far as I know. But I don't think we really need required kwargs in Sound#play so a simple hash sounds fine as in Image.from_text sounds good enough :)

Another random thought. We should test whether play_pan(0) and play sound the same with a stereo sample, not just good old Beep.wav :)

Kjarrigan · 2018-03-13T15:32:04Z

test/test_audio.rb


-    song.play(true)
+    channel = song.play(looping: true)


Really not sure of this, but most folks already adapted the Sample.play = Channel logic, so why not make it the same for "Songs"

... and preparations before actually rewrite the logic behind.

jlnr · 2018-03-13T21:52:22Z

And one more thought. Right now Gosu reserves one dedicated OpenAL channel for songs. That prevents a situation where Song.play silently fails when many samples are playing already.

One clean solution would be to pass an optional priority into every play call and samples with a higher priority can kick samples with a lower priority out even while they're playing. But...I guess we can push this issue back until someone actually runs into this situation :)

jlnr · 2018-05-05T13:54:45Z

Another thing that I'll have to rethink with this PR is the "automagic handling" of Gosu::Song in the iOS/UIKit port:

https://github.com/gosu/gosu/blob/master/src/GosuViewController.cpp#L108-L128

Both the code and logic are ancient. I need to work on that part anyway because it's using deprecated APIs.

mblumtritt · 2018-05-05T17:21:24Z

lib/gosu/compat.rb

+  end
+
+  def play(looping=true)
+    @channel = super(looping: true)


Why is the original parameter ignored here?

Kjarrigan added 2 commits March 13, 2018 16:01

Changed the Audio.hpp to the targeted interface

ba17c1a

Change Tests to reflect the new interface as well

f11564f

Kjarrigan added audio enhancement labels Mar 13, 2018

Kjarrigan commented Mar 13, 2018

View reviewed changes

Just a little more changes ...

26e13b9

... and preparations before actually rewrite the logic behind.

mblumtritt reviewed May 5, 2018

View reviewed changes

lib/gosu/compat.rb

end

def play(looping=true)

@channel = super(looping: true)

Copy link

mblumtritt May 5, 2018

Choose a reason for hiding this comment

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

Why is the original parameter ignored here?

jlnr force-pushed the master branch 3 times, most recently from 05ac06d to 46cd8fd Compare October 4, 2018 19:54

jlnr mentioned this pull request May 13, 2020

Feature Request: Create Sample from Array or binary String #198

Open

This was referenced Nov 15, 2020

Song/Sample#length [on hold] #527

Closed

OSX Yosemite error with long songs. #301

Closed

jlnr marked this pull request as draft December 13, 2020 22:57

jlnr force-pushed the master branch from da38bc1 to 813d852 Compare December 29, 2020 22:45

jlnr force-pushed the master branch from 5f9a3d8 to 6312a09 Compare March 20, 2021 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Audio 2.0 #441

[WIP] Audio 2.0 #441

Kjarrigan commented Mar 13, 2018

Kjarrigan Mar 13, 2018

jlnr Mar 13, 2018

Kjarrigan Mar 13, 2018

jlnr Mar 13, 2018

Kjarrigan Mar 13, 2018

jlnr Mar 13, 2018

Kjarrigan Mar 13, 2018

jlnr Mar 13, 2018

Kjarrigan Mar 13, 2018

jlnr Mar 13, 2018

Kjarrigan Mar 13, 2018

Kjarrigan Mar 13, 2018

jlnr Mar 13, 2018

Kjarrigan Mar 13, 2018

Kjarrigan Mar 13, 2018

jlnr Mar 13, 2018 •

edited

jlnr Mar 13, 2018 •

edited

Kjarrigan Mar 13, 2018

jlnr Mar 13, 2018 •

edited

jlnr Mar 13, 2018

Kjarrigan Mar 13, 2018

jlnr commented Mar 13, 2018

jlnr commented May 5, 2018

mblumtritt May 5, 2018

[WIP] Audio 2.0 #441

Are you sure you want to change the base?

[WIP] Audio 2.0 #441

Conversation

Kjarrigan commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlnr Mar 13, 2018 • edited

Choose a reason for hiding this comment

jlnr Mar 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlnr Mar 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlnr commented Mar 13, 2018

jlnr commented May 5, 2018

Choose a reason for hiding this comment

jlnr Mar 13, 2018 •

edited

jlnr Mar 13, 2018 •

edited

jlnr Mar 13, 2018 •

edited