Rewrite sf::Event API to improve type safety

[ChrisThrasher]

Description

Closes #2668

This new API is built on top of std::variant. This allows us to store many different event types in a space-efficient way and access the active event type in a type-safe manner that eliminates the types of UB that are possible with unions.

I tried to find docs that referenced the old API but may have missed a few places. Initially my focus is on consensus for API design. After that we can focus harder on updating all the docs. There are also some missing comment blocks I need to fill in and more tests to write so there is some finishing work to do.

---

What are all these new structs you added?

The current implementation has fewer structs than values in the enumeration. This means there is not a 1-to-1 mapping between event types and data structures. By ensuring there is a 1-to-1 mapping it lets us use the variant index to get the enumeration value. This is very easy to write, read, and maintain. Believe me this is a lot better than alternative implementations that don't add new structs.

What about std::visit?

I'm reluctant to add this because adding an element to a variant constitutes an API break for anyone visiting that variant. You must visit a variant with the perfect set of callbacks for the elements in the variant. If we allow users to visit this variant then we're effectively preventing ourselves from adding new events in a given major release. For now I'd prefer to retain that ability. We can always add this later if desired.

EDIT 28 Jan: Apparently it is possible to support std::visit without prohibiting us from adding new events so I'm open to this idea.

Why bother keeping the enumeration in the API instead of the alternatives in #2668?

I highly value letting users continue to use switch to process potential event states. I think that's a very useful, powerful way to list all the actions one may take in response to a given event type. It's also nice to know that users don't have to remove the switch statements they've been writing for years. While the syntax for event processing are different, a lot of their existing knowledge can be carried forward. Users can still write if/else if chains if they prefer but they're not forced to.

Invalid variant access results in an exception being thrown. We should not let exceptions escape/this leaks implementation details.

I don't see this as a problem. Any exceptions only occur in incorrect programs so no one writing correct code will ever deal with them. The exceptions will server to very quickly catch bugs in user programs. To avoid leaking this detail would require we jump through extra hoops and possibly introduce new avenues for UB that I would rather not do.

---

EDIT 16 Apr 2024: The rest of the team disagrees about the value of returning an enumeration so I removed the APIs that used the enum. I still stand by this as a good API but don't want to stall this PR indefinitely so I removed it.

[codecov]

Codecov Report

Attention: Patch coverage is 29.71888% with 175 lines in your changes are missing coverage. Please review.

Project coverage is 44.61%. Comparing base (6eaf300) to head (bdeaae0).
Report is 23 commits behind head on master.

❗ Current head bdeaae0 differs from pull request most recent head 0423c45. Consider uploading reports for the commit 0423c45 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2766      +/-   ##
==========================================
+ Coverage   43.65%   44.61%   +0.95%     
==========================================
  Files         253      255       +2     
  Lines       20940    21132     +192     
  Branches     5139     5212      +73     
==========================================
+ Hits         9142     9427     +285     
+ Misses      10785    10711      -74     
+ Partials     1013      994      -19     

Files	Coverage Δ
include/SFML/Graphics/RenderWindow.hpp	66.66% <ø> (ø)
include/SFML/Window/Event.inl	100.00% <100.00%> (ø)
include/SFML/Window/Window.hpp	0.00% <ø> (ø)
include/SFML/Window/WindowBase.hpp	0.00% <ø> (ø)
src/SFML/Window/Event.cpp	100.00% <100.00%> (ø)
src/SFML/Window/macOS/SFOpenGLView.mm	60.90% <100.00%> (ø)
src/SFML/Window/macOS/Scaling.h	87.50% <ø> (+0.83%)	⬆️
include/SFML/Window/Event.hpp	97.61% <97.61%> (-2.39%)	⬇️
src/SFML/Window/WindowBase.cpp	64.25% <0.00%> (-0.10%)	⬇️
src/SFML/Window/iOS/WindowImplUIKit.mm	0.00% <0.00%> (ø)
... and 9 more

... and 58 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

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

[ChrisThrasher]

I contained the templates so you can't just pass any random template argument into it.

[ChrisThrasher]

I removed sf::Event::set since it's just unnecessary and I'd rather not have too many ways of doing the same thing. I added a non-const sf::Event::get since otherwise manipulating the data in-place is not possible. You'd have to copy it out first before modifying it then reassigning it into the event.

[Bromeon]

Thanks a lot for writing this PoC! 💪

Looking at the event handling,

switch (event.type) 
{
    case sf::Event::Type::KeyPressed:
        if (event.get<sf::Event::KeyPressed>().code == sf::Keyboard::Escape)
            window.close();
        break;
    case ...
}

I wonder if we could consider some alternative idioms to reduce repetition a bit?
Example:

if (auto* key = event.as<sf::Event::KeyPressed>()) {
        if (key.code == sf::Keyboard::Escape)
            window.close();
}
// no break needed

else ...

or even:

if (auto* key = event.keyPressed(); key && key.code == sf::Keyboard::Escape)
     window.close();
}
// no break needed

else ...

This would also avoid the age-old type safety problem: you cannot accidentally check for type X and process Y. The concrete type is directly returned.

[ChrisThrasher]

I'm fine adding an analog to std::get_if which would do what you're looking for. It's only a few lines of code and very easy to test and maintain as well. Would that work?

[ChrisThrasher]

I added sf::Event::getIf and found some places internally to use it which help prove its value. If you're already using if statements for event handling then this is a very useful addition.

I'm still questioning whether I want to keep the non-const overloads of get and getIf. I can't find any use cases within this repo for using them.

[ChrisThrasher]

https://github.com/ChrisThrasher/mandelbrot/compare/event_api

Quick experiment trying the new API in a project of mine. I love how I can still use a switch. The changes to my code are not drastic yet I'm getting a huge upgrade in type safety.

[ChrisThrasher]

I pushed a commit that improves error messages. Previously you'd get a failed static_assert alongside a lot of std::variant template errors which were not necessary to understand the problem. Now thanks to if constexpr we can skip all those template errors and go straight to the failed static_assert. Using Clang 17 on my machine this results in about 1/5th as many lines of error text in the console.

[ChrisThrasher]

SFML/imgui-sfml@master...event_api

Here is what it looks like to apply this API to ImGui-SFML. Writing this uncovered a potential shortcoming of this API

        case sf::Event::Type::KeyPressed: // fall-through
        case sf::Event::Type::KeyReleased: {
            const bool down = (event.type == sf::Event::KeyPressed);

            const ImGuiKey mod = keycodeToImGuiMod(event.key.code);
            // The modifier booleans are not reliable when it's the modifier
            // itself that's being pressed. Detect these presses directly.
            if (mod != ImGuiKey_None) {
                io.AddKeyEvent(mod, down);
            } else {
                io.AddKeyEvent(ImGuiKey_ModCtrl, event.key.control);
                io.AddKeyEvent(ImGuiKey_ModShift, event.key.shift);
                io.AddKeyEvent(ImGuiKey_ModAlt, event.key.alt);
                io.AddKeyEvent(ImGuiKey_ModSuper, event.key.system);
            }

            const ImGuiKey key = keycodeToImGuiKey(event.key.code);
            io.AddKeyEvent(key, down);
            io.SetKeyEventNativeData(key, event.key.code, -1);
        } break;

In the event where fall through behavior is desired, this new API requires duplicating the code inside each case or moving the code out into a function template.

The only solution I can think of (other than having users simply deal with this) is to put both press and release events into one event with a new field for disambiguating between whether the key was pressed or released. I'm not sure I like this though.

[Bambo-Borris]

Pointed one of my personal projects towards the event_api branch to try out these changes locally. Didn't notice any issues with mouse/keyboard events on X11. Had to change very little existing code, it is essentially like working with the old form. I found myself using the sf::Event::getIf method as it aligned with the existing event handling code I had.

For someone used to SFML I think this kind of API change will be mostly unnoticed but still yielding better type safety. For new users I think this poses no more diffficulty to grasp than the old form, but with the benefit of better type safety. Think this is a great PoC for showing how SFML could modernise events, hope to see it get further along and potentially become the new way in SFML3.

[ChrisThrasher]

I took the remaining events that include x, y, and z fields and converted them to instead hold an sf::Vector. This has had downstream simplifying effects on anything using those events.

[ChrisThrasher]

Periodic rebase. No changes made.

[coveralls]

Pull Request Test Coverage Report for Build 8864601618

Details

• 65 of 228 (28.51%) changed or added relevant lines in 12 files are covered.
• 21 unchanged lines in 6 files lost coverage.
• Overall coverage increased (+0.3%) to 51.888%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/SFML/Window/WindowBase.cpp	0	2	0.0%
src/SFML/Window/macOS/SFOpenGLView+keyboard.mm	0	2	0.0%
src/SFML/Window/macOS/SFKeyboardModifiersHelper.mm	0	4	0.0%
src/SFML/Window/macOS/SFOpenGLView+mouse.mm	0	4	0.0%
src/SFML/Window/WindowImpl.cpp	0	8	0.0%
src/SFML/Window/macOS/WindowImplCocoa.mm	7	26	26.92%
src/SFML/Window/DRM/InputImpl.cpp	0	26	0.0%
src/SFML/Window/Unix/WindowImplX11.cpp	1	45	2.22%
src/SFML/Window/Win32/WindowImplWin32.cpp	2	56	3.57%

Files with Coverage Reduction	New Missed Lines	%
src/SFML/Window/macOS/SFKeyboardModifiersHelper.mm	1	33.96%
src/SFML/Window/Win32/WindowImplWin32.cpp	1	30.56%
src/SFML/Window/DRM/InputImpl.cpp	2	0.0%
src/SFML/Window/Unix/WindowImplX11.cpp	3	43.24%
src/SFML/Window/macOS/WindowImplCocoa.mm	4	46.64%
src/SFML/Window/macOS/SFOpenGLView+keyboard.mm	10	6.49%

Totals
Change from base Build 8863316403:	0.3%
Covered Lines:	10468
Relevant Lines:	18998

---

💛 - Coveralls

[eXpl0it3r]

API Design

From an API design point of view I have three comments:

• I do agree that having multiple ways to do the same-ish thing is bad and can/will cause a lot of confusion
• Adding APIs is a non-breaking change, as such I propose to pick one API for SFML 3 and if we notice a lot of ergonomic issues, we always have the possibility to expand the API surface in SFML 3.1. This of course would prevent certain nearly "free" upgrade paths from SFML 2 to 3.
• getIf<T>() is in my opinion not a simple API. It's not very complex either, but it would be one of the first and very central APIs that will require some understand of templates. And while C++ dev familiar with std::variant will be used to get_if it's really not a great speaking API. We're going from having to know if/switch/enums to if/"templates"/"variants"/optional. I wonder if there's a way to provide the "advanced" API in addition to a more speaking/simpler API, close to what @Bromeon proposed - consider how we provide Vector2f usings for the same simplicity in usage.

Switch Ergonomics

Personally, I've often fallen back to if & else if blocks, mostly to circumvent the non-exhaustive enum/missing default case warnings. Given that for small projects (most common type of SFML projects) you rarely will handle all events and given that enabling these warnings are generally recommended, it disarms the argument in favor of switch-case ergonomics for me quite a bit.
Keep in mind though, that I've a rather limited experience with C++, so maybe this view doesn't line up with what others experience.

My Proposal

Seeing most of the voice speaking up for support of getIf<T> and the points listed above, I would propose to go with getIf<T> only in this PR as a first step.

I'd then be very interested in seeing potential proposals to "simplify" the API.

[ChrisThrasher]

I removed getType and get from the API.

[ChrisThrasher]

Incorporated Kimci's review comments and rebased on master.

[ChrisThrasher]

Rebased on master and fixed merge conflicts.

[MarioLiebisch]

Just as a random thought that just occurred to me: Since we're handling mouse position anyway to track inside/outside, I'd consider MouseEntered and MouseLeft to be MouseMoved events that should report a position, if supported.

[Bromeon]

This has been open for a long time and discussed even longer before that 🙂
I think it's in a good state to be merged; if there are more things to refine, they could be done as follow-up PRs.

Thanks for the initiative, as well as all the continuous updates and rebases @ChrisThrasher! 👍

[ChrisThrasher]

Rebased on master since some new clang-tidy checks were added. No changes made.

[eXpl0it3r]

I don't think, this will be the final form of the API for SFML 3, but let us collect some more feedback from people using it, by getting it into master.

Thank you for all the work put into @ChrisThrasher and all the discuss around it so far from everyone! I believe it's a good thing that we the "old" API is being challenged and forces us to reflect some more about the design and safety.