Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: replace relative system/speed scale with user-defined transform callback #11539

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

expikr
Copy link
Contributor

@expikr expikr commented Nov 26, 2024

This is an accompanying Proof-of-Concept for #11449, requesting discussion on what to do with the relative speed scale hint (relative system scale hint should be replaced outright regardless):

Option 1: remove relative speed scale (this implementation)

This is my preference, as the scaling functionality can easily be folded into the developer-defined function. and the main problem raised in the issue is that developer may be surprised by the end-user being able to alter the behavior from environment variables, so it is necessary to move the control exclusively to the developer instead.

Option 2: apply relative speed scale if no custom transform

This is essentially the inverse of the current behavior but with custom rather than system transform. The end user alterable relative speed scale taking a lower precedence than developer-controlled transform function removes a large portion of the element of surprise for the developer. However, the issue might still exist for developers that want specific scaling values and expected to use the events system exclusively without knowing about this new callback functionality.

Option 3a: apply relative speed scale before custom transform

Essentially, this is a philosophical assertion that the end-user wants to have precedence over the behavior, that the inputs are uniformly scaled according to their specification before the developer code is made aware of it.

Option 3b: apply relative speed scale after custom transform

I cannot think of any good reason to do this, considering that this causes a divergence between what the developer expected from their transform output and when they receive the event by inserting an end-user configurable step in between.

@expikr expikr force-pushed the transform-function branch 8 times, most recently from 2cbc0c0 to 0b4cc83 Compare November 26, 2024 15:47
@expikr expikr force-pushed the transform-function branch from 75232a6 to 5ebda77 Compare December 19, 2024 07:12
@expikr
Copy link
Contributor Author

expikr commented Dec 19, 2024

Hmm, TIL Wayland sends accelerated and unaccelerated counts in the same message, so passing the default system transform might not make a lot of sense. Gonna think about how custom transform should fit in.

@Kontrabant
Copy link
Contributor

Kontrabant commented Dec 19, 2024

The backend could just pass a dummy function pointer for the system transform function, which, if set, tells the event callback to pass through the system accelerated values.

@expikr
Copy link
Contributor Author

expikr commented Dec 20, 2024

So something like:

typedef struct {
    float pending_scaled_dx;
    float pending_scaled_dy;
} Wayland_MouseData;

static Wayland_MouseData Wayland_system_scale_data;

void Wayland_OnMouseMessage(float rawX, float rawY, float scaleX, float scaleY) {
    Wayland_system_scale_data.pending_scaled_dx = scaleX;
    Wayland_system_scale_data.pending_scaled_dy = scaleY;
    SDL_SendMouseMotion(rawX, rawY);
}

static void Wayland_ApplySystemScale(void *internal, Uint64 timestamp, SDL_Window *window, SDL_MouseID mouseID, float *x, float *y) {
    Wayland_MouseData *data = internal;
    *x = data->pending_scaled_dx;
    *y = data->pending_scaled_dy;
    data->pending_scaled_dx = 0;
    data->pending_scaled_dy = 0;
}

void Wayland_InitMouse(SDL_VideoDevice *_this) {
    SDL_Mouse *mouse = SDL_GetMouse();
    mouse->ApplySystemScale = Wayland_ApplySystemScale;
    mouse->system_scale_data = &Wayland_system_scale_data;
    ...
}

@Kontrabant
Copy link
Contributor

Was thinking of something simpler, like this: Kontrabant@3e3dd6d

It just uses the system transformation function pointer as a flag for whether to pass accelerated or unaccelerated data, and the function itself is a no-op.

@slouken slouken added this to the 3.x milestone Dec 21, 2024
@expikr
Copy link
Contributor Author

expikr commented Dec 21, 2024

While this proposal pitches a deferable candidate solution, its lemmatic questions should be settled before the release of 3.2.0:

  • are Relative SystemScale/MultiplierScale Hints considered an irrevocable API/ABI committment?
  • are the current form of this interface ruling out certain solutions in the future/painting ourselves into a corner?
  • are there any middleground options that lets us avoid commitment for the time-being?

Considerations that may factor into the answers to the above:

  • is the GDC talk considered a committing promise?
  • what are the current state of utilization of these two hints from the:
    • end-user side, i.e. paranoid CS player config cargo cult
    • end-developer side, i.e. are they depending on it as a functionality, or treating them as something to be mitigated? A quick search on GitHub showed some emulator devs trying it out, finding it janky, and decides to do their own scaling instead.
    • library binding side
  • to what extent are certain API or ABI breakages acceptable within this major version?

@slouken
Copy link
Collaborator

slouken commented Dec 24, 2024

  • are Relative SystemScale/MultiplierScale Hints considered an irrevocable API/ABI committment?

No, hints are not part of the ABI/API commitment, however SDL changes should not completely break applications that use them. For example, if we don't use system scale, but provide a reasonable approximation, or implement the hint via new APIs we provide, that's fine.

  • are the current form of this interface ruling out certain solutions in the future/painting ourselves into a corner?

If by the current form, you mean the hints, the answer is no, see above.

  • are there any middleground options that lets us avoid commitment for the time-being?

I think any middle ground changes are likely to paint us into a smaller corner later, because we need to support everything that is in the current API going forward.

  • is the GDC talk considered a committing promise?

No.

  • what are the current state of utilization of these two hints from the:

    • end-user side, i.e. paranoid CS player config cargo cult

Excellent question. I don't have any data on this, but if users are relying on these hints as-is, then we will need to support them going forward.

  • end-developer side, i.e. are they depending on it as a functionality, or treating them as something to be mitigated? A quick search on GitHub showed some emulator devs trying it out, finding it janky, and decides to do their own scaling instead.

I think this is a pretty likely state of affairs.

  • library binding side

Library bindings should not be relying on hints.

  • to what extent are certain API or ABI breakages acceptable within this major version?

API and ABI breakages are not allowed. Hints can be added or removed as long as they do not completely break games in the process.

@expikr expikr force-pushed the transform-function branch 2 times, most recently from d2b0c54 to 8025fb6 Compare January 11, 2025 18:59
@slouken
Copy link
Collaborator

slouken commented Jan 11, 2025

We should not remove the hints, please remove that from your PR.

I wonder if we're overthinking this here. The application can do any transform they want after they get the raw deltas. The only transform that is useful is the one to match system mouse acceleration curves, which can be dynamic, and only matters if the application is enabling relative mouse motion and drawing their own cursor.

@expikr
Copy link
Contributor Author

expikr commented Jan 11, 2025

The central issue is the lack of a guarantee for the developers that the deltas received in events are raw. This proposal would be fully supplanted if the relative mode state machine is frozen as a quarantined dinosaur and an alternate raw stream is added instead.

Also, tangential to this point, I believe there is currently no way for developers to force System Scale even with the highest priority set if the user had set a relative speed scale, as it has a higher precedence and there is no such thing as "unset hint with priority".

@slouken
Copy link
Collaborator

slouken commented Jan 11, 2025

The central issue is the lack of a guarantee for the developers that the deltas received in events are raw. This proposal would be fully supplanted if the relative mode state machine is frozen as a quarantined dinosaur and an alternate raw stream is added instead.

The deltas received in events should be considered raw when relative motion is enabled, and are raw in as many cases as possible. It is more likely that we'll re-add raw motion events in the future than change how this works so close to official SDL3 release.

Also, tangential to this point, I believe there is currently no way for developers to force System Scale even with the highest priority set if the user had set a relative speed scale, as it has a higher precedence and there is no such thing as "_un_set hint with priority".

That's a good point. System scale should have higher priority than linear scale and that's a change we can make now.

@expikr
Copy link
Contributor Author

expikr commented Jan 11, 2025

(cutting the following into separate a comment instead of editing since you already replied to the above)

and only matters if the application is enabling relative mouse motion and drawing their own cursor.

Taking a step back, the bigger problem I think is the fact that two distinct concepts are combined into one in SDL, with no way to control their behavior separately: the concept of a software cursor and the concept of relative motion.

SDL's software cursor position cannot be influenced by transforms in the event loop, it can only be modified by the SystemScale/MultiplierScale hints.

But if you set those hints, now the relative deltas received in the event loop are also modified, so you lose the original unscaled deltas.

@expikr
Copy link
Contributor Author

expikr commented Jan 12, 2025

That's a good point. System scale should have higher priority than linear scale and that's a change we can make now.

Or, if not going with the custom transform, it should first apply system scale as pre-scale and multiplier scale as post-scale. Can hints be renamed?

@slouken
Copy link
Collaborator

slouken commented Jan 12, 2025

Can hints be renamed?

No, but you can add a separate hint that has the same functionality. It's probably not worth it at this point.

@expikr
Copy link
Contributor Author

expikr commented Jan 12, 2025

I've opened #11921 for the above smaller-scoped change.

@slouken
Copy link
Collaborator

slouken commented Jan 12, 2025

I've opened #11921 for the above smaller-scoped change.

lol, I already added it in 551510c

@expikr
Copy link
Contributor Author

expikr commented Jan 12, 2025

The order is wrong, it should be applied after, not before, otherwise the speed calculation is completely off.

@slouken
Copy link
Collaborator

slouken commented Jan 12, 2025

The order is wrong, it should be applied after, not before, otherwise the speed calculation is completely off.

It's done that way for consistency with Wayland, which passes through the scaled values. What's the impact of that?

@expikr
Copy link
Contributor Author

expikr commented Jan 12, 2025

It's done that way for consistency with Wayland, which passes through the scaled values.

That doesn't make sense to me, Wayland passing scaled values means that it should in principle only be possible to apply sequentially as a post-scale, no?

What's the impact of that?

Because the acceleration curve is nonlinear, applying it beforehand means that you are changing where it falls on the domain of the transform function. Applying it afterwards instead makes it a uniform scaling.

@slouken
Copy link
Collaborator

slouken commented Jan 12, 2025

It's done that way for consistency with Wayland, which passes through the scaled values.

That doesn't make sense to me, Wayland passing scaled values means that it should in principle only be possible to apply sequentially as a post-scale, no?

Yes. /facepalm. I'll accept your PR, thanks!

@expikr expikr force-pushed the transform-function branch from 8025fb6 to 7a951d7 Compare January 13, 2025 06:06
@expikr
Copy link
Contributor Author

expikr commented Jan 13, 2025

I'm getting conflicting signals here about the compatibility constraints of our solution space, my understanding is:

  • ABI/API are strictly forbidden from changing, existing compiled binaries should be able to function with a drop-in replacement.
  • Hints can be added/removed/changed as long as the functionality is provided, it is up to the developer to migrate.
  • introducing new API should be deliberated thoroughly and is not appropriate too close to release
  • Hints serve as a way to tentatively test new functionality without commitment, until it gets de-facto adoption at which point the functionality needs to be at least be available in some form.

This was the reason why the previous PR was opened offering to remove to speed scale hint, as it is de-factor not really used and SDL3 offers an opportunity to overhaul the system, the conservative option is to remove it first since it is a hint not subject to ABI lock (and there have been many clunky hints present in SDL2 but removed in SDL3 even after the ABI-stable preview)

I'm not trying to argue for one option or another, it's just that it's currently very confusing with seemingly contradictory feedbacks about what are the available options.

@slouken
Copy link
Collaborator

slouken commented Jan 13, 2025

I'm getting conflicting signals here about the compatibility constraints of our solution space, my understanding is:

  • ABI/API are strictly forbidden from changing, existing compiled binaries should be able to function with a drop-in replacement.

True

  • introducing new API should be deliberated thoroughly and is not appropriate too close to release

True

  • Hints can be added/removed/changed as long as the functionality is provided, it is up to the developer to migrate.
  • Hints serve as a way to tentatively test new functionality without commitment, until it gets de-facto adoption at which point the functionality needs to be at least be available in some form.

This is not true. Hints are a way of providing SDL a hint for behavior that the application desires. The behavior may not be available on all platforms and there's no formal API for what the hint is affecting. How important the hint is depends on how it affects application behavior, and to some extent the current state of release.

In this case, since we are coming right up on release, we have basically locked down and are not making any significant API or behavior changes to SDL3.

@expikr expikr marked this pull request as draft February 6, 2025 10:21
@expikr expikr force-pushed the transform-function branch from c745b47 to c54e219 Compare February 7, 2025 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants