search

KelvinShadewing / brux-gdk

Free runtime and development kit using SDL and Squirrel
GNU Affero General Public License v3.0
39 stars 20 forks source link

Rework audio portability API to use shared objects instead of handling at compile time #72

Open tulpenkiste opened 1 month ago

tulpenkiste commented 1 month ago

This is a dependency of issue #69.

This has benefits such as:

tulpenkiste commented 1 month ago

Gonna do some changes to xySetAudioDriver quickly

tulpenkiste commented 1 month ago

Ok, ready for review I think?

hexaheximal commented 1 month ago

This pull request's implementation is fundamentally problematic.

After reading it, I can think of a few issues off the top of my head:

I can see why you would want to do this, but, as much as I hate to say it, the way you chose to do it is one of the worst possible ways to implement it.

Honestly, if you really wanted, I could just make an implementation myself which solves all of those problems and more. And one of these days I probably should get around to finishing what I started here...

On Thursday, May 23rd, 2024 at 6:08 PM, Tulpen @.***> wrote:

Ok, ready for review I think?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.

tulpenkiste commented 1 month ago
hexaheximal commented 1 month ago

The getAudioDriver() name change was to make the function name generally make more sense as it felt fairly ambiguous.

I guess that makes some sense.

What would the benefits of dynamic loading be?

However, again, not every platform supporting dynamic linking and/or dynamic loading. You're defeating the entire purpose of the portability API by adding it in its current form.

The drivers are in C++ classes because thats the most convenient and effective way for me to write it. We already have a bunch of C++ classes in brux, I don't see the problem.

NO. BAD. STOP. We should be using less classes (let alone class inheritance), NOT more. You can literally just use function pointers alone to do the exact same thing you are trying to do, and it's A LOT more straightforward.

From what I can recall, you came from the KDE/Qt world. I came from Go, and I know from experience that they did a good job of solving these problems.

You may have noticed that Go doesn't have class inheritance, or even classes at all. There's a reason for that.

In the world of Go, the problems with traditional OOP are well-known.

Typically, class inheritance is abused as an easy way to get multiple implementations of the same method schema - which is exactly what you're doing.

In the world of Go, this is done in a much more elegant way: Define an interface which defines what methods should be on an implementing struct, define a struct implementing it (you can put methods on Go structs too!), and then pass in the struct to a function which takes that interface as an input.

See also:

On Thursday, May 23rd, 2024 at 9:01 PM, Tulpen @.***> wrote:

  • The getAudioDriver() name change was to make the function name generally make more sense as it felt fairly ambiguous.
  • What would the benefits of dynamic loading be? Dynamic linking already widely supported and we generally need something that doesn't cause problems when we eventually add SDL3 support.
  • The drivers are in C++ classes because thats the most convenient and effective way for me to write it. We already have a bunch of C++ classes in brux, I don't see the problem.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

sussycatgirl commented 1 month ago

So basically you're calling her out for doing something that is bad practice in a completely different programming language that has absolutely nothing to do with C++?

Mind not being condescending about other people's contributions based on your personal preferences?

hexaheximal commented 1 month ago

So basically you're calling her out for doing something that is bad practice in a completely different programming language that has absolutely nothing to do with C++?

I'm not mentioning Go just because I like it - I'm mentioning it because the way they solved it is actually relevant here. And that's far from the only thing I could mention here which already solved these problems and are relevant.

Mind not being condescending about other people's contributions based on your personal preferences?

This isn't about personal preferences. This is about NOT KILLING BRUX GDK.

I genuinely really like Brux GDK and I want it to succeed. Artificially limiting the engine to only run on platforms which implement dynamic linking and/or dynamic loading won't get us there.

I created the audio portability API (see: https://github.com/KelvinShadewing/brux-gdk/commit/4b18f9040afe6e882ac6e9115df7d12c84447dc5) with a specific goal in mind: Make it possible to run Brux GDK on more niche platforms that don't support SDL2. The original reason for this was a libctru-based 3DS port.

The operating systems of nearly every game console do not support dynamic linking, let alone dynamic loading. Switching to dynamic linking and/or dynamic loading would single-handedly break that.

Despite everything, nobody even bothered to ask the Brux GDK / SuperTux Advance community in general if this would cause any issues. And even if that wasn't done, you could still look at the history of how the portability API came to be, which would make the problems obvious.

Dynamic linking is not a good idea for Brux GDK, but dynamic loading (as in dlopen etc.) would make it easier to write IO backend code. Brux GDK should really just do something like what the Linux kernel did with kernel modules.

Honestly, I'm seriously starting to consider forking Brux GDK and/or making a compatible re-implementation at this point...

On Friday, May 24th, 2024 at 11:52 AM, Lea @.***> wrote:

So basically you're calling her out for doing something that is bad practice in a completely different programming language that has absolutely nothing to do with C++?

Mind not being condescending about other people's contributions based on your personal preferences?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

KelvinShadewing commented 1 month ago

@hexaheximal I mean, why not implement your own dynamic loading and do a PR? Show us how you'd tackle this issue. It hasn't been merged, so it's still open to alternative solutions. I'm certainly curious to see it myself.

tulpenkiste commented 1 month ago

So basically you're calling her out for doing something that is bad practice in a completely different programming language that has absolutely nothing to do with C++?

I'm not mentioning Go just because I like it - I'm mentioning it because the way they solved it is actually relevant here. And that's far from the only thing I could mention here which already solved these problems and are relevant.

How in the god damn fuck is it relevant? It's a completely different programming language with completely different standards for doing things? It's like if I went to swap a GPU and you disagree and pull out a sound card. You aren't making any sense here.

Mind not being condescending about other people's contributions based on your personal preferences?

This isn't about personal preferences. This is about NOT KILLING BRUX GDK. I genuinely really like Brux GDK and I want it to succeed. Artificially limiting the engine to only run on platforms which implement dynamic linking and/or dynamic loading won't get us there. I created the audio portability API (see: 4b18f90) with a specific goal in mind: Make it possible to run Brux GDK on more niche platforms that don't support SDL2. The original reason for this was a libctru-based 3DS port. The operating systems of nearly every game console do not support dynamic linking, let alone dynamic loading. Switching to dynamic linking and/or dynamic loading would single-handedly break that. Despite everything, nobody even bothered to ask the Brux GDK / SuperTux Advance community in general if this would cause any issues. And even if that wasn't done, you could still look at the history of how the portability API came to be, which would make the problems obvious. Dynamic linking is not a good idea for Brux GDK, but dynamic loading (as in dlopen etc.) would make it easier to write IO backend code. Brux GDK should really just do something like what the Linux kernel did with kernel modules. Honestly, I'm seriously starting to consider forking Brux GDK and/or making a compatible re-implementation at this point...

I will be making it so that it can statically link 1 non-null audio backend in a future PR (specifically, the multi renderer backend PR). We didn't ask the STA community about this because.... everyone uses a computer for STA? Brux's joystick control is very annoying still so we shouldn't even be targeting consoles until thats sorted out.

Additionally, you still haven't really provided a good reason for why dynamic loading is better. From what I see, dynamic loading would worsen the code drastically due to needing a bunch of boilerplate that can be avoided by using dynamic linking on desktop.

As for you forking, if it means you won't be rude to people who are contributing to free software in their spare time for not doing things the way YOU want, then go ahead, leave us be.

KelvinShadewing commented 1 month ago

@hexaheximal Couldn't you just statically link what you need for the 3DS port and dynamic linking could be used generally?

tulpenkiste commented 1 month ago

@hexaheximal Couldn't you just statically link what you need for the 3DS port and dynamic linking could be used generally?

He could. I'll be making this possible on my next PR