Fix polyphonic audio streams with id > 1 cannot be stopped or changed (MSVC mis-optimization)

[ditiem-games]

Fixes #86053

[Chaosus]

This is not an acceptable name for the commit/PR. Please rename it to meaningful one (like "Fix audio streams with id > 1 cannot be stopped or changed") and squash the commits.

[bruvzg]

If this logic is removed, then these two lines in the header should be removed as well:

godot/scene/resources/audio_stream_polyphonic.h

Lines 59 to 60 in 475248d

	ID_MASK = 0xFFFFFFFF,
	INDEX_SHIFT = 32

Why was it saving/using index in the first place? It might be performance critical, and in this case it's better to keep it, check index first and then fallback to the full list search instead of removing it completely.

[ditiem-games]

If this logic is removed, then these two lines in the header should be removed as well:
Agree.

Why was it saving/using index in the first place?

I thought about that for quite a while and my only conclusions was that whoever did it consider some use cases but forgot the delete song case or maybe in the moment it was implemented that case was not possible.

It might be performance critical, and in this case it's better to keep it, check index first and then fallback to the full list search instead of removing it completely.

We need serious data for this, and I personally do not have it. I thought of the cases about how I did/will use it in my games and how it will be impacted. So my assumptions were:

1. There are not gonna be many songs, around 5 is the max I got (32 is the max allowed if I am not mistaken).
2. It is not very usual to call this function (just when stopping, updating the pitch/volume by some fade-out/in, for instance).
3. By the moment that you end with the checks of whether the "id" was correct and you can "optimise" the search, you are probably in the 3rd of 4th song using the for implementation. But for this affirmation, deep testing is really needed, and I do not know how to do that in the current project. If this point is true, accessing, in my cases, will take in average double the time.

If you want to reduce the linear access time, a hash function or AVL tree (or similar) should be used instead.

Is there any data about number of streams and how often are they accessed given the id?

[ditiem-games]

Why was it saving/using index in the first place?

There is a second approach that can be taken which I think it is interesting. The streams vector:

godot/scene/resources/audio_stream_polyphonic.h

Line 78 in 475248d

LocalVector<Stream> streams;

contains elements of type Stream. Here the rules:

1. It should be possible to create some "Null Stream" element that will substitute a deleted element, instead of collapsing the vector.
2. Whenever a new Stream is added, the "Null Stream" elements are checked first to use that index in the ID
3. The current implementation of index + stream-id will remain as it is.
4. Vector may shrink if all the end elements are "Null Stream", but I am not sure it is worth the shrinking.

Creation will be linear, but all the rest will be constant access.

I will be away for a week and I will give it a try on my return.

[ditiem-games]

The idea in the previous comment is exactly what the author had in mind. So the bug was much deeper.

The ID_MASK was defined as 0xFFFFFFFF, which was treated as an int so it was extended to 0xFFFFFFFFFFFFFFFF, failing to extract the low 32 bits of a uint64_t.

I would have personally used an struct with two uint32_t and avoid all these masks.

There was another issue that was not addressed. When the stream ended, it was not freed until another stream will take its slot, if that ever happened. I do not know if this was intended. I refactored the code to unref the stream.

[ditiem-games]

@bruvzg @Chaosus guys, can you have a look at this PR? It is ready for merge, the fix was way simpler at the end and nothing gets changed or broken.

[akien-mga]

See also #86054 for an alternative approach.

[akien-mga]

I slightly prefer #86054 as it's more minimal and prevents some extra casting (it seems weird to have a int64_t id but then always cast it internally to uint64_t before casting back to int64_t), but both solutions were valid. Thanks for the contribution!

[ditiem-games]

Using a class enum was the first thing that came to my mind. But there are two other issues that are still open:

1. The enum is exported in the .h file unnecessarily because no other file uses it.
2. The ID was already declared as an int. I tried changing it to uint, but it breaks some APIs. So, I went with casting as the only alternative left. It should be removed by the compiler in most cases, but if that's not the case and things are promoted to int, the bug remains.

Anyway, as long as the bug works on all the platforms of my game, I am happy with it!

[akien-mga]

The enum is exported in the .h file unnecessarily because no other file uses it.

That's a common practice in the Godot codebase to put helper constants like this in an enum. But indeed it does lead to issues like this one with MSVC being peculiar with enums, so it doesn't hurt to refactor it on a case by case basis.

The ID was already declared as an int. I tried changing it to uint, but it breaks some APIs. So, I went with casting as the only alternative left. It should be removed by the compiler in most cases, but if that's not the case and things are promoted to int, the bug remains.

Yeah there's no uint in the scripting API, int in GDScript is signed 64-bit. So I guess casting like you did does make sense.

Let's go with this approach then :)

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[Zireael07]

It's probably worth documenting in the code itself why it's done this way