Remove device indices, only use instances

[icculus]

So a thing we do in several places in SDL is use an index:

int total = SDL_GetNumAudioDevices(0);
for (int i = 0; i < total; i++) {
    printf("Audio device #d: %s\n", SDL_GetAudioDeviceName(0, i));
}

When opening a device, we open it by this index number, and in return we are given a device ID (audio), or instance (joystick), which has no further association with the index number.

Device index can change as devices come and go, as can the total number of devices, so things that take the index need to be stable until the next call to SDL_GetNumAudioDevices(), and locks need to be held in various places to make sure the index doesn't change in the middle of using it.

When events references these devices, their which field is the device instance, which might be confusing if all you had was an index number and an SDL_Joystick *...now you're scrambling to figure out what this number means, or worse, just assuming it was the index.

So let's get rid of the indexes.

Let's have one call that fills a buffer with instance IDs, which are associated with a device for as long as they exist (opened or not), while SDL is initiailized. We already track these.

So that for loop would become:

int total;
SDL_AudioDeviceID *devs = SDL_GetAudioDeviceList(0, &total);
for (int i = 0; i < total; i++) {
    printf("Audio device #%d: %s\n", (int) devs[i], SDL_GetAudioDeviceName(0, devs[i]));
}
SDL_free(devs);

So if this list changes during the for-loop, SDL will notice the instance is not a currently-valid item and return a failure (NULL in this case), but life will continue.

devs can be NULL, and SDL_free will do the right thing with it (and total will be zero here).

We delete all the things that are named SDL_JoystickGetWidgetForIndex(int index).

There's more to this, but that's the idea.

[slouken]

One of the advantages of this approach is that in cases where locking is needed, like joysticks, etc. the entire list can be returned atomically.

A couple more comments on details: In general these APIs should be returning IDs of some sort, where 0 is an invalid ID. The list is then guaranteed to be 0 terminated, allowing the caller to pass NULL for the count and do something like this:

SDL_AudioDeviceID *devs = SDL_GetAudioDeviceList(0, NULL);
if (devs) {
    for (int i = 0; devs[i]; i++) {
        printf("Audio device #%d: %s\n", (int) devs[i], SDL_GetAudioDeviceName(0, devs[i]));
    }
    SDL_free(devs);
}

This semantic also guarantees that returning NULL means an internal error occurred, as distinct from a list with only 0 in it, indicating an empty list.

We should double check with @attila-lendvai and folks on https://github.com/libsdl-org/SDL/issues/6337 to see if this maps well to language bindings.

[eloj]

Are you sure it's not worth making these accept memory to fill out, rather than doing an internal heap allocation?

This would make the process less atomic, but not in a way that'd be very harmful that I can see; i.e at the point of filling in the memory, the returned enumeration is always equally valid in both designs.

// Modelled on Khronos-style enumerations, e.g
// https://registry.khronos.org/OpenCL/sdk/1.0/docs/man/xhtml/clGetPlatformIDs.html
int num_sensors;
int err = SDL_GetSensors(0, NULL, &num_sensors); // Get count

// Implies num_sensors > 0
if (err == SDL_OK) {
    SDL_SensorID *sensors = whatever_alloc(num_sensors, sizeof(*sensors));
    SDL_GetSensors(num_sensors, sensors, NULL); // Fill out memory
    whatever_free(sensors);
}

I can certainly see arguments against, just raising this to make sure it's been considered and rejected before the API gets locked down.

[slouken]

Yes, we considered that pattern. Microsoft also uses that for many APIs. The problem is that it's not atomic, and you always risk the count changing between the two calls. The recommended pattern for robustness with that style of API is to actually do the allocations in a loop:

SDL_SensorID *sensors = NULL;
int num_sensors;
int result;
while ((result = SDL_GetSensors(0, sensors, &num_sensors)) == BUFFERTOOSMALL) {
    SDL_SensorID *new_sensors = (SDL_SensorID *)realloc(sensors, num_sensors * sizeof(*sensors));
    if (new_sensors == NULL) {
        /* Handle out of memory condition */
        if (sensors) {
            free(sensors);
        }
        return;
    }
    sensors = new_sensors;
}
if (result == OK) {
    int i;
    for (i = 0; i < num_sensors; ++i) {
        /* Do something with sensors[i] */
    }
}
if (sensors) {
    free(sensors);
}

So, since we already provide memory allocation functions, we decided to make this much easier to use correctly. :)

[icculus]

Okay, this is in for audio devices now; sensors, joysticks, gamepads, and display modes were already done.

Things still using this pattern:

• Haptics (are we keeping this API?)
• Gamepad Mappings (SDL_GetNumGamepadMappings/SDL_GetGamepadMappingForIndex)
• Gamepads don't use this, but there's a bunch of \sa things in the header for a non-existent SDL_GetGamepadNameForIndex

[slouken]

Okay, this is in for audio devices now; sensors, joysticks, gamepads, and display modes were already done.

Things still using this pattern:

• Haptics (are we keeping this API?)

The jury is out on this one. It's useful for applications that want to use the phone vibration service. It's also useful for games that want to support wheels and haptic flightsticks. I'm tempted to just remove gamepad rumble from the interface and leave it. Thoughts?

• Gamepad Mappings (SDL_GetNumGamepadMappings/SDL_GetGamepadMappingForIndex)

This can stay as-is. Mappings don't have an ID, and this allows enumerating them.

• Gamepads don't use this, but there's a bunch of \sa things in the header for a non-existent SDL_GetGamepadNameForIndex

Fixed!

[icculus]

The jury is out on this one. It's useful for applications that want to use the phone vibration service. It's also useful for games that want to support wheels and haptic flightsticks. I'm tempted to just remove gamepad rumble from the interface and leave it. Thoughts?

I guess the question is: is there modern support for this, or is it stuck on a DirectInput 8 interface that never got upgraded?

(And if it never got upgraded, is that because DI8 was Good Enough, or was it where the industry decided this was too niche to continue to support?)

If we're keeping it, I think we definitely make rumble a separate thing. That's what most things want, it's what modern APIs support. Right now the Windows haptic stuff has this weird split between DirectInput8 and XInput, and I suppose that's just to support rumble, and it might be nice to chop that out if nothing else.

[slouken]

I'm going to close this and open a separate issue to figure out what to do with haptics.