WIP: Move semantics for `SFML/Audio`

[vittorioromeo]

This needs testing and work, but I want to get some early feedback on the direction of this changes and the implementation details.

[codecov[bot]]

Codecov Report

Merging #2011 (0901d58) into master (5b500ad) will increase coverage by 0.69%. The diff coverage is 33.93%.

@@            Coverage Diff            @@
##           master   #2011      +/-   ##
=========================================
+ Coverage    6.55%   7.24%   +0.69%     
=========================================
  Files         182     183       +1     
  Lines       15765   15907     +142     
  Branches     4076    4107      +31     
=========================================
+ Hits         1033    1153     +120     
- Misses      14603   14622      +19     
- Partials      129     132       +3     

Impacted Files	Coverage Δ
include/SFML/Audio/SoundBuffer.hpp	0.00% <ø> (ø)
src/SFML/Audio/Sound.cpp	0.00% <0.00%> (ø)
src/SFML/Audio/SoundBuffer.cpp	0.00% <0.00%> (ø)
src/SFML/Audio/SoundRecorder.cpp	0.00% <0.00%> (ø)
src/SFML/Audio/SoundSource.cpp	0.00% <0.00%> (ø)
src/SFML/Audio/AlResource.cpp	88.31% <86.15%> (+75.81%)	:arrow_up:
src/SFML/Audio/ALCheck.cpp	8.57% <0.00%> (+8.57%)	:arrow_up:
src/SFML/Audio/AudioDevice.cpp	25.51% <0.00%> (+25.51%)	:arrow_up:
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5b500ad...0901d58. Read the comment docs.

[binary1248]

What was the rationale behind introducing a new type just to manage a buffer ID? Wouldn't it be more elegant to wrap the entire buffer into its own type and move that around?

[vittorioromeo]

What was the rationale behind introducing a new type just to manage a buffer ID? Wouldn't it be more elegant to wrap the entire buffer into its own type and move that around?

That would be a possible approach, I was weary of having to basically implement a wrapper over OpenAL primitives.

[binary1248]

I think that is something that is necessary to abstract/translate things from the non-C++ world into the C++ world.

Many of the lower level APIs are for (IMHO) good reason C APIs. This allows them to stay stable (and ABI stable) for a very long time.

Because our goal is to provide an abstraction over these APIs that is C++-native we will not get around writing "wrapper-like" code to manage their resources. The question is whether each resource type should get its own wrapper that we can use/re-use all over the place, or if the wrapping functionality should be spread out to wherever these resources are used by other types.

Currently the latter is the case, however as can be seen in the current situation, adding support for modern C++ semantics increases the boilerplate so substantially that it makes more sense now than it did pre-modern-C++ to focus the wrapping into specialized wrapper types.

Providing specialized wrapper types also has the added benefit that future features will be able to re-use the existing infrastructure thus reducing the cost of their implementation and that potential bugs can be fixed with minimal effort. These types don't even have to be a part of the public interface, reserving us the capability to adjust them internally when necessary without having to break the public API.

I'm normally not a proponent of the "everything is a class" camp (looking at you Java), but this is one of those rare cases where it really does make sense.

[vittorioromeo]

Because our goal is to provide an abstraction over these APIs that is C++-native we will not get around writing "wrapper-like" code to manage their resources. The question is whether each resource type should get its own wrapper that we can use/re-use all over the place, or if the wrapping functionality should be spread out to wherever these resources are used by other types.

But we already achieve our goal with sf::SoundBuffer, sf::SoundSource, et cetera. Are you suggesting that SFML should become a de-facto C++ wrapper over OpenAL?

Currently the latter is the case, however as can be seen in the current situation, adding support for modern C++ semantics increases the boilerplate so substantially that it makes more sense now than it did pre-modern-C++ to focus the wrapping into specialized wrapper types.

Providing specialized wrapper types also has the added benefit that future features will be able to re-use the existing infrastructure thus reducing the cost of their implementation and that potential bugs can be fixed with minimal effort. These types don't even have to be a part of the public interface, reserving us the capability to adjust them internally when necessary without having to break the public API.

If you want to go down that route, it makes sense to me to start a separate library that wraps the entirety of OpenAL into nice reusable abstraction. I'm not saying it's a bad idea, but I believe it is outside the scope of "adding move semantics to sf::SoundBuffer".

[binary1248]

sf::SoundBuffer, sf::SoundStream etc. do much more than just manage underlying OpenAL resources, they also manage interdependencies between themselves and other objects, that is currently where their complexity is concentrated. It would make it much easier to maintain and reason about these classes if we could separate these concerns. This is basically the same reason why smart pointers have become the only reasonable way to manage memory.

What we are discussing here doesn't only apply to sfml-audio, it would also apply to sfml-graphics as well since we have essentially the same "problem" there.

SFML is (at least from my perspective as an OpenGL programmer) already sort of a wrapper library for certain OpenGL resources (textures, shaders, etc.). These are the building blocks that the library needs to provide higher levels of abstraction which themselves can be built on the lower native C++ abstractions of these primitives.

I don't think it would make sense to extract this functionality into a separate library. It is one of the core value propositions that SFML currently provides.

Following the mantra of "you only pay for what you use" these wrappers will be available to developers. If they choose to live purely in a world of higher abstraction they should be able to do so. Want to write some lower level stuff yourself? Sure, go ahead. It shouldn't prevent us from making use of such types to make our lives easier implementing said higher level abstractions.

SFML has for a while had a reputation of being beginner friendly at the expense of not being powerful enough to more experienced users. It would be nice if we could use this opportunity to widen the perceived target audience of the library.

I do share your opinion that adding wrapper types might be out of scope of just adding move semantics, but that begs the question: Why not shift the focus to implementing move-aware wrappers from the start? I personally would find it a waste of effort implementing something knowing that it will become obsolete in a very short period of time.

[ChrisThrasher]

I'm implementing std::experimental::unique_resource in this branch. I think it's a promising option for wrapping these owning resource handles such that we can drastically reduce how often we have to explicitly mention copy and move operations and can instead just let those get implicitly defined.