Bug in the Audio Module with listeners?

[Tank]

LOL

[Tank]

To push this discussion here's an idea (I haven't thought about naming much, that's more up to the people who know how "newbie-friendly naming" works -- for me "forward and up vectors" make perfect sense and are easily understandable -- at last chance give the people an illustration and they'll get it immediately, I'm sure):

class Listener {
        public:
                struct Direction {
                        Direction( sf::Vector3f forward, sf::Vector3f up );

                        sf::Vector3f mForward;
                        sf::Vector3f mUp;
                };

                void setDirection( Direction );
                Direction getDirection() const;

        private:
                Direction mDirection;
};
 

That way you can't change any of both values independently and you are very near the current API (those functions could be even provided as overloads for adding it in 2.2).

Another option would be to rename "setDirection" to "pointAt" or "lookAt" or "listenAt" or even "listenTo" (native English speakers please). Same structure applies.

Edit: therocode suggested to use "orientation" instead of "direction". I agree to that.

[therocode]

Yes, I too would agree that Tank's API proposal is nice. I also agree on the fact that "up" is as semantically descriptive as "at". "at" is where the listener looks at, and "up" is what is up to the listener. Can't get more simple than that I think.

[Nexus]

Direction won't be changed. Up is a separate attribute (and making a structure would break existing code anyway).

The API would rather look as follows:

class Listener
{
public:
     static void setDirection(const Vector3f& direction);
     static void setUp(const Vector3f& up);

     static Vector3f getDirection();
     static Vector3f getUp();
};

And yes, there might be a more expressive name than "up" (or "upVector"). But it seems like "up" is commonly used.

[Laurent]

I'm not a native english speaker, but with this API I immediately thought about this

sf::sleep(sf::hours(7));
sf::Listener::getUp();

[Nexus]

Indeed also "setUp" could be confused with "setup".

Maybe "set/getUpVector" or "set/getUpward" or similar. But also something different (e.g. referring to the listener's head, such as overhead, if it just didn't have this ambiguous meaning) would be possible.

[Lo-X]

Get up, stand up
Stand up for your rights

Isn't it possible to have a overload or a new setDirection like

void sf::Listener::setDirection(const Vector3f& direction, const Vector3f& up = sf::Vector3f(0, 1, 0))

?

[Nexus]

Of course it would be possible, but it's a very unintuitive API because:

• setDirection should set the direction and only that
  
• There can't be a corresponding getDirection
  
• Everybody needs to read the documentation to find out why there are two parameters

I don't understand why everybody wants to mix two attributes.

[Lo-X]

Hum, you're right it doesn't make sense

[Tank]

Direction won't be changed. Up is a separate attribute

I don't understand why everybody wants to mix two attributes.

In this case "up" shouldn't be seen as a separate attribute, because the forward and up vectors together make the orientation, and they are highly dependent on each other. For properly setting an orientation you have to provide both -- they both form the data type. (Compare it to sf::Color for example: r, g, b and a all form the compound "Color"; having sf::Sprite::setR(), sf::Sprite::setG() etc. would not make sense)

(and making a structure would break existing code anyway).

Could we please put that argument aside? I think you should also consider a solution that breaks the current (and broken, by the way) API. We don't want another WinAPI, do we?

[Nexus]

In this case "up" shouldn't be seen as a separate attribute, because the forward and up vectors together make the orientation, and they are highly dependent on each other.

The same applies for texture and texture rect, view center and view size, and so on. There are many examples where you have to call multiple methods to set the correct state.

Could we please put that argument aside? I think you should also consider a solution that breaks the current (and broken, by the way) API.

No. It can be discussed for SFML 3, but until then there won't be breaking changes. I don't know why this has to be stated again and again...

I listed three points why the proposed API is unintuitive (to which you didn't respond). On the other side, there is a very clean solution to add an up vector to the existing API.

[Tank]

The same applies for texture and texture rect, view center and view size, and so on. There are tons of examples where you have to call multiple methods to set the correct state, and I see no reason to deviate from that scheme.

Then those should change as well, thanks for pointing them out. It's like your often mentioned RAII: Why allow dangerous situations if they can be avoided by design?

The problem I see with such things is that it's easy to use them wrong. And in case of SFML especially, it even remains silent if you do so (no assert, no exception, no message) -- or even does not allow to use it right, like this particular audio bug.

No. It can be discussed for SFML 3, but until then there won't be breaking changes. I don't know why this has to be stated again and again...

I don't care for what version, I try to find a good solution. This shouldn't have to do something with any version number at all: There's a bug, there should be a clean fix.

2.x can live with a workaround, 3 should have a clean solution. Personally I would go with a clean solution immediately, because I don't find backwards-incompatible API changes problematic (if they happen in 2.2 or 3.0 does not really matter in my opinion -- sooner or later it will happen, so why postponing it for an unnecessary long time?).

I've personally never heard something like "Oh no, those API changes are so massive, I will stop using SFML because of it!". (Except the naming scheme change, which surely pissed off some people. ) It's not like that it takes ages to adjust code to work with newer versions. And you have always the option to stay with an older version if it works for you.

I listed three points why the proposed API is unintuitive (to which you didn't respond). On the other side, there is a very clean solution to add an up vector to the existing API.

I did not respond because I did not propose it, you replied to Lo-X.

[Nexus]

I don't consider separate methods for up vectors a workaround, it's very probable that it would have been done so in the first place. It's a common way of providing access to attributes, but of course there may be different opinions.

The problem with your API (which is almost the same as Lo-X', as there would also be a setDirection overload with 2 parameters) is that it's not very intuitive. You have to know all the involved types exactly, while the API with separate methods allows people knowing the OpenAL concept, but not SFML, to work directly with SFML's listeners.

Compare the following codes:

sf::Listener::setOrientation(sf::Listener::Orientation(a, b));
// vs.
sf::Listener::setDirection(a);
sf::Listener::setUpVector(b);

Which one is more expressive and easier to read?

Even if you provided a direct overload to allow the call

sf::Listener::setOrientation(a, b);

it is not clear what a and b mean. It's not simply done by naming the variables differently, because often you work with temporary expressions.

[Laurent]

Then those should change as well, thanks for pointing them out.

No they shouldn't. I can move my view without changing its size, or I can zoom it without changing its position. I can change a texture while keeping the same rect, and I can change the rect within the same texture. Same for the listener orientation: in most cases (all 2D and most 3D) the up vector will be fixed to +Y and never change after that. So let's use a new property, which works well, is consistent with the current API, and has no constraint. A good documentation will then do the rest.

I don't care for what version, I try to find a good solution.

It's important to have in mind if a modification has to be scheduled for SFML 3, or can/must be applied immediately. The design considerations are not the same.

Personally I would go with a clean solution immediately, because I don't find backwards-incompatible API changes problematic (if they happen in 2.2 or 3.0 does not really matter in my opinion -- sooner or later it will happen, so why postponing it for an unnecessary long time?).

I really don't want to start discussing the benefits of versioning rules, I think you're smart and experienced enough to understand why they are needed, let's focus on the initial issue please.

[Tank]

For me setOrientation() is easier to read, because it's what's more natural. I want to change the orientation, so I directly set it, not its component. I also rather do object.setPosition() instead of object.setX(), object.setY() etc. Or object.setColor() instead of object.setRedColor(), object.setGreenColor() etc. Orientation is a compound of forward and up vectors, that's how it works. I think you can't abstract it furthermore.

Also the concept of having a forward and up vector is nothing OpenAL invented or which is typical for it, it's basically mathematics, a way to express an orientation.

The problem with your separated functions is that the user has to know that he has to call two functions in order to correctly set the orientation. Auto completion heroes might simply stop by setDirection() and call it. In 2D environments in might even work if the up vector is by default set to {0, 1, 0}. But they completely miss the concept of how positioning the listener really works.

So, even if the compound is not intuitive (which it is in my eyes though), it at least makes sure that the user has to face the "forward/up vector" thing. Without knowing it I'd bet that the most will try to find out what it's all about. Result is: Lesson learned, no bug, everybody's happy.

The argument that it's not clear what specific arguments mean is not valid in my opinion. At first you say that you don't know what "a" and "b" means. Yes, that's right, if you name the parameters like that, then you should consider choosing a more meaningful name. And second, the same could apply to setDirection(): Is the parameter an angle? A relative vector? Absolute vector? Polar coordinate?

In the end the user has to learn the concept behind the code. Then using the code is as easy as a) using auto completion or b) using Doxygen.