Remove default empty state of `sf::Event` #2992

vittorioromeo · 2024-05-11T13:57:29Z

Proof of concept right now as I don't have access to a compiler, but I think this is a superior design compared to #2991.

Removing the empty state from event results in a much more type-safe API. Events cannot be default-constructed anymore, and APIs that poll events now return an optional event, making it clear that an empty state can be present without polluting usages of events that cannot be in such an empty state.

@ChrisThrasher: feel free to continue working on this branch to adjust the usages of event in tests and examples.

ChrisThrasher · 2024-05-11T16:50:28Z

I considered this in the past and decided I didn’t like it because it requires excessive operator-> usage. I find the current operator bool API more ergonomic even if it’s less ideologically pure.

ChrisThrasher · 2024-05-11T16:58:44Z

master...optional_events

Here's my branch from back in November. I looked at this and didn't think it was worth doing. To avoid lots of operator-> usage I had to create a new intermediate variable that unwrapped the optional.

coveralls · 2024-05-13T17:53:51Z

Pull Request Test Coverage Report for Build 9465514141

Details

5 of 56 (8.93%) changed or added relevant lines in 4 files are covered.
5 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.04%) to 55.715%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/SFML/Window/DRM/WindowImplDRM.cpp	0	2	0.0%
src/SFML/Window/WindowBase.cpp	5	10	50.0%
src/SFML/Window/WindowImpl.cpp	0	17	0.0%
src/SFML/Window/DRM/InputImpl.cpp	0	27	0.0%

Files with Coverage Reduction	New Missed Lines	%
include/SFML/Window/WindowBase.hpp	1	0.0%
src/SFML/Window/WindowBase.cpp	2	74.09%
src/SFML/Window/DRM/InputImpl.cpp	2	0.0%

Totals
Change from base Build 9463943212:	-0.04%
Covered Lines:	11561
Relevant Lines:	19663

💛 - Coveralls

L0laapk3 · 2024-05-13T19:21:08Z

What are the advantages of this over std::monostate?

vittorioromeo · 2024-05-13T22:45:20Z

What are the advantages of this over std::monostate?

Monostate is just another symbol for ab empty state, which is a state a valid event should never be in.

As @ChrisThrasher mentioned in his PR, event shouldn't even have an empty state and most importantly such a state should not be public and accessible to users.

The reason why we had an empty state to begin with is because some APIa such as pollEvent need to return immediately and tell the user that no event was polled.

In C++17 we have an idiomatic way of expressing the possible absence of a result: optional. We are using that throughout the library already and using it for events solves the "public empty state but not really" issue and, even better, makes event a type that's always guaranteed to be valid and non empty.

L0laapk3 · 2024-05-13T23:05:25Z

... since users already should not be using sf::Event::Empty.

@ChrisThrasher Can you explain the rationale behind this? What is wrong with sf::Event::Empty? Is it just about having to implement similar safeties to std::monostate?

vittorioromeo · 2024-05-13T23:10:46Z

... since users already should not be using sf::Event::Empty.

@ChrisThrasher Can you explain the rationale behind this? What is wrong with sf::Event::Empty? Is it just about having to implement similar safeties to std::monostate?

It's a meaningless implementation detail. Operating systems or windowing systems do not produce empty events. We simply needed a sentinel to signal "hey, you tried to poll events but nothing was available" in user loops.

vittorioromeo · 2024-06-04T17:23:17Z

@eXpl0it3r @binary1248: could you please express your thoughts on this PR? It follows exactly the same philosophy as these already-merged PRs:

And the same philosophy as the soon-to-be-merged PRs:

Remove default empty state of sf::Texture #3027

In fact, I don't understand @ChrisThrasher's concerns given the fact that he's the author of all the PRs mentioned above.

It is true that the user experience gets a tiny bit worse due to the fact that the -> token must be used compared to the . token, and due to the fact that the user needs to be familiar with std::optional, but I genuinely believe that is a blessing in disguise.

Our users' will benefit from the type safety of std::optional, and once they learn the pattern of using it, they will appreciate the consistency of such pattern being used everywhere in the library, sf::Event included.

There are a few more benefits to this PR:

We avoid exposing the sf::Event::Empty class publicly to the users, which was just an implementation detail;
We avoid a public operator bool conversion on sf::Event, which is again an implementation detail exposed to the user, as we were pretty much reimplementing a std::optional on our own;
sf::Window::waitEvent now guarantees to return a non-empty event, simplifying code that uses waitEvent instead of pollEvent;
The signature of sf::Window::pollEvent now clearly shows that an event might not be returned, making it clear how it differs from waitEvent and alerting users that they need to check for the empty case;
The concerns regarding the syntactical change from . to -> brought by this PR are very nicely
addressed by a proposed declarative visitation-based event processing technique, see Implement sf::Event::visit #3015 and Exploration of sf::Event::visit use cases #3014. See below for a code example.

In conclusion, I strongly believe that this PR helps us fulfill our goal of making invalid states unrepresentable in the type system. Not merging it would not only hinder us from reaching that goal, but would undermine the work done by @ChrisThrasher with his previously merged PRs due to a glaring inconsistency between most SFML types and sf::Event.

--

EDIT: I also wanted to point out that by merging this PR and #3015, @ChrisThrasher's concern about the -> syntax is removed if a visitation-based API is used:

pollEvents(window, 
    [&](sf::Event::Closed)
    { 
        window.close(); 
    },
    [&](sf::Event::KeyPressed e)
    {
        if (e.code == sf::Keyboard::Key::Escape)
            window.close();
        else if (e.code == sf::Keyboard::Key::Space)
            playAction();
    },
    [&](sf::Event::TouchBegan)
    {
        playAction();
    }
    [&](sf::Event::Resized)
    {
        sf::View view;
        view.setSize({gameWidth, gameHeight});
        view.setCenter({gameWidth / 2.f, gameHeight / 2.f});
        window.setView(view);
    }
);

ChrisThrasher · 2024-06-04T17:29:02Z

sf::Window::waitEvent now guarantees to return a non-empty event, simplifying code that uses waitEvent instead of pollEvent;

The signature of sf::Window::pollEvent now clearly shows that an event might not be returned, making it clear how it differs from waitEvent and alerting users that they need to check for the empty case;

This is a good point I had not considered. I don't like the idea of having two functions with the same signature with different API promises about whether or not an event is returned.

This PR opens the door to a very nice declarative visitation-based event processing technique, see Implement 'sf::Event::visit' #3015 and Exploration of sf::Event::visit use cases #3014.

Can you elaborate on how this PR relates to supporting visitation?

vittorioromeo · 2024-06-04T17:36:10Z

Can you elaborate on how this PR relates to supporting visitation?

I overstated my point, the same visitation API can be achieved even without this PR. The only difference is that without this PR, we'd have to inject a sf::Event::Empty overload ourselves to make the visitor passed to std::visit exhaustive.

This PR removes the need for such a "hack", sort of proving that everything fits better together.

Edited my original post.

ChrisThrasher

I suppose I can come around to this change. It is technically correct after all. My main argument about ergonomics is hard to support. Hopefully I'm wrong and users won't complain too much about using -> so much in their event loops.

Still curious to hear from the rest of the team.

ChrisThrasher · 2024-06-04T17:34:29Z

.github/ISSUE_TEMPLATE/bug_report.yml

@@ -44,8 +44,8 @@ body:
          label: Steps to reproduce
          description: Tell us how to reproduce this issue and provide a [minimal, complete and verifiable example](https://stackoverflow.com/help/mcve), you can use the template below
          value: |
-              1. 
-              2. 


I think these trailing newlines and the other trailing newlines in this file may be intentional

Trailing spaces are intentional, so you can start typing directly

Will fix when I get back home

ChrisThrasher · 2024-06-04T17:34:48Z

.github/ISSUE_TEMPLATE/bug_report.yml

@@ -57,10 +57,13 @@ body:

                  while (window.isOpen())
                  {
-                      while (const auto event = window.pollEvent())
+                      while (const std::optional event = window.pollEvent())


As a point of style I'd prefer we continue to use auto here and in all other similar instances

We're basically doing that, only that we're constraining the type inference to a std::optional of some T. I think it's worthwhile using std::optional here instead of auto to clearly suggest that an event might or might not be polled.

I considered two alternatives:

auto: hides too much information, not clear that a std::optional is returned;

std::optional<sf::Event>: too verbose and redundant, the information about being an event is already clearly available from the name of the variable event.

Using std::optional strikes a good balance between conciseness and providing useful information to the reader.

Before bringing up the same discussion point in every PR, maybe let's leave it as it is right now. Start a separate discussion and the align the code pieces afterwards.

It's not helpful when someone writes code in one way and someone else then goes and keeps changing it afterwards in their way.

That's fair, but there was a reason I made this change specifically in this PR, as it was now unobvious that pollEvent returns an optional with plain auto.

ChrisThrasher · 2024-06-04T17:36:11Z

.github/ISSUE_TEMPLATE/feature_request.yml

                              window.close();
+                              break;


I suppose this break ensures that event processing ceases when a close is requested? In theory this reduces the latency between clicking the close button the application actually closing. Is that your intent?

That's not my intent, but it's a nice side benefit of the change.

The intention is to prevent event processing from causing issues when using waitEvent, as waitEvent now asserts m_impl != nullptr, but WindowBase::close immediately invokes m_impl.reset().

We need to call break; to avoid accessing a deleted m_impl on the next iteration.

Now that I wrote this, it does sound like a subtle trap if the break; is forgotten... But the only way I can think of fixing this is to revert waitEvent to return a std::optional<sf::Event>, which loses a nice API improvement that this PR would provide.

I think a decent alternative would be to do this in waitEvent:

Event WindowBase::waitEvent() { if (m_impl == nullptr) { sf::err() << "waitEvent invoked on destroyed window -- did you forget a `break`?" << std::endl; std::terminate(); } std::optional<sf::Event> event = m_impl->popEvent(true /* blocking */); assert(event.has_value()); filterEvent(*event); return *event; }

I've added the additional safety measure to waitEvent and updated its documentation.

Yeah, calling std::terminate() is never an acceptable solution in my book.

Not sure if using an default constructed Closed event is really the solution. We've found an edge case where waitEvent can return an empty state, as such I feel like it should be represented in the API. waitEvent could return false in SFML 2, we shouldn't pretend that this state doesn't exist anymore in SFML 3.

We've found an edge case where waitEvent can return an empty state

It's more of a precondition failure -- the precondition is that waitEvent should not be called on a window that's already been closed. Returning a sf::Event::Closed is a way of not terminating nor getting UB, but it's not behavior the user should rely on.

Perhaps I can add an err() before the return to make it clear that the user is doing something wrong and we're just being nice?

Actually, this just reminded me of the longstanding issue of waitEvent never returning if there no more events are being produced. For example if you run waitEvent in a separate thread and in the other thread closes the window. The event thread will continuing spinning as it doesn't get any events to process: https://github.com/orgs/SFML/projects/4?pane=issue&itemId=23482732# / #1184

Since SFML is anyways doing a spinning while loop, due to Joystick event processing, I'd actually propose to change waitEvent to take a custom timeout parameter. Blocking methods should always have a way a cancellation token or timeout option.

In that case you'll have another situation where waitEvent has a "failure case".

The user isn't doing anything wrong, when calling any of the window methods after closing the window. Because you still have a valid object and as such any operation on that object should remain valid for the lifetime of the object.

I like the timeout idea, I will work on a PR for that specifically (against master) and then integrate that into this one as they are separate concerns.

Created PR for the timeout here: #3094

It's more of a precondition failure -- the precondition is that waitEvent should not be called on a window that's already been closed.

In my opinion calling waitEvent on a closed window is precondition violation. We should log an error, assert(false), then return sf::Event::Closed to ensure behavior is defined in release builds. I still want that function to return an sf::Event rather than std::optional<sf::Event>.

I prefer we not add this break everywhere. Let's only add it when waitEvent is being used. Doing so will reduce the amount of code this PR touches and make me more eager to merge it.

I strongly disagree. As long as an object was initialized, it's in a valid state. We cannot suddenly have functions semi-fail because we assign random preconditions.

Besides, with the timeout proposal, you actually get an expected failure case.

I'm also against adding break; everywhere.

My views:

It's very unfortunate that we lose the nice non-optional API for waitEvent if we add the timeout, but I think it's a good idea to have one;

I agree with @ChrisThrasher that calling waitEvent on a closed window should be a precondition violation and I agree with his proposed solution -- however if we do add a timeout it would make sense to return std::nullopt there as well;

I disagree with both of you regarding break; -- I think that the only sensible thing to do when you close a window is to break out of the event handling loop, and I'd like to promote that practice regardless of whether or not it's strictly necessary for the correctness of the program.

include/SFML/Window/WindowBase.hpp

src/SFML/Window/Android/WindowImplAndroid.cpp

src/SFML/Window/DRM/InputImpl.cpp

src/SFML/Window/WindowBase.cpp

src/SFML/Window/WindowImpl.cpp

src/SFML/Window/WindowImpl.hpp

binary1248 · 2024-06-04T18:10:41Z

People can't simultaneously praise other hyped up languages for their at times really esoteric syntax and at the same time complain that C++ isn't staying up to date while complaining when ever so often we use already existing language constructs to make the language "less bad" than what its undeserved reputation has many laypeople believe.

Sure I also prefer having to type less, but not at the cost of higher mental load because I have to think of something that wouldn't have been necessary if I would have typed a bit more. I prefer typing over thinking if there was a choice.

I have used SFML for more than a decade at this point, and never have I spent more than 1% of total project time messing with event handling. If anyone spends so much time on event handling that this actually makes a difference they are doing something wrong anyway.

src/SFML/Window/WindowBase.cpp

examples/raw_input/RawInput.cpp

src/SFML/Window/WindowBase.cpp

vittorioromeo requested review from ChrisThrasher, Bromeon and binary1248 May 11, 2024 13:57

vittorioromeo mentioned this pull request May 11, 2024

Implement the empty event state with std::monostate #2991

Closed

vittorioromeo requested a review from eXpl0it3r May 13, 2024 13:48

eXpl0it3r added feature m:sfml-window labels May 14, 2024

eXpl0it3r added this to the 3.0 milestone May 14, 2024

vittorioromeo mentioned this pull request May 15, 2024

Exploration of sf::Event::visit use cases #3014

Open

3 tasks

vittorioromeo force-pushed the feature/remove_empty_event branch from d9de771 to 2953f26 Compare May 19, 2024 22:27

vittorioromeo force-pushed the feature/remove_empty_event branch from 2953f26 to 887eff1 Compare June 2, 2024 18:06

vittorioromeo changed the title ~~Remove 'Empty' state from 'Event', rely on 'optional'~~ Remove default empty state of sf::Event Jun 4, 2024

vittorioromeo force-pushed the feature/remove_empty_event branch from 887eff1 to 695c250 Compare June 4, 2024 16:58

ChrisThrasher requested changes Jun 4, 2024

View reviewed changes

vittorioromeo force-pushed the feature/remove_empty_event branch from 695c250 to 3addaed Compare June 5, 2024 01:06

vittorioromeo commented Jun 5, 2024

View reviewed changes

src/SFML/Window/WindowBase.cpp Outdated Show resolved Hide resolved

vittorioromeo commented Jun 5, 2024

View reviewed changes

examples/raw_input/RawInput.cpp Outdated Show resolved Hide resolved

vittorioromeo force-pushed the feature/remove_empty_event branch from dab07c6 to 8411668 Compare June 5, 2024 14:17

vittorioromeo commented Jun 5, 2024

View reviewed changes

src/SFML/Window/WindowBase.cpp Outdated Show resolved Hide resolved

vittorioromeo requested a review from ChrisThrasher June 5, 2024 14:20

vittorioromeo mentioned this pull request Jun 7, 2024

Remove default empty state of sf::Music #3075

Merged

ChrisThrasher mentioned this pull request Jun 10, 2024

Remove default empty state of sf::FileInputStream #3088

Merged

vittorioromeo force-pushed the feature/remove_empty_event branch from 8411668 to 82e883b Compare June 10, 2024 22:53

Remove default empty state of sf::Event

f66cef1

vittorioromeo force-pushed the feature/remove_empty_event branch from 82e883b to f66cef1 Compare June 11, 2024 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove default empty state of `sf::Event` #2992

Remove default empty state of `sf::Event` #2992

vittorioromeo commented May 11, 2024

ChrisThrasher commented May 11, 2024

ChrisThrasher commented May 11, 2024 •

edited

coveralls commented May 13, 2024 •

edited

L0laapk3 commented May 13, 2024

vittorioromeo commented May 13, 2024

L0laapk3 commented May 13, 2024 •

edited

vittorioromeo commented May 13, 2024

vittorioromeo commented Jun 4, 2024 •

edited

ChrisThrasher commented Jun 4, 2024 •

edited

vittorioromeo commented Jun 4, 2024

ChrisThrasher left a comment

ChrisThrasher Jun 4, 2024 •

edited

eXpl0it3r Jun 11, 2024 •

edited

vittorioromeo Jun 11, 2024

ChrisThrasher Jun 4, 2024

vittorioromeo Jun 5, 2024

eXpl0it3r Jun 11, 2024

vittorioromeo Jun 11, 2024 •

edited

ChrisThrasher Jun 4, 2024

vittorioromeo Jun 5, 2024

vittorioromeo Jun 5, 2024

eXpl0it3r Jun 11, 2024

vittorioromeo Jun 11, 2024

vittorioromeo Jun 11, 2024

vittorioromeo Jun 11, 2024

ChrisThrasher Jun 12, 2024 •

edited

eXpl0it3r Jun 12, 2024

vittorioromeo Jun 12, 2024

binary1248 commented Jun 4, 2024

Remove default empty state of sf::Event #2992

Are you sure you want to change the base?

Remove default empty state of sf::Event #2992

Conversation

vittorioromeo commented May 11, 2024

ChrisThrasher commented May 11, 2024

ChrisThrasher commented May 11, 2024 • edited

coveralls commented May 13, 2024 • edited

Pull Request Test Coverage Report for Build 9465514141

Details

💛 - Coveralls

L0laapk3 commented May 13, 2024

vittorioromeo commented May 13, 2024

L0laapk3 commented May 13, 2024 • edited

vittorioromeo commented May 13, 2024

vittorioromeo commented Jun 4, 2024 • edited

ChrisThrasher commented Jun 4, 2024 • edited

vittorioromeo commented Jun 4, 2024

ChrisThrasher left a comment

Choose a reason for hiding this comment

ChrisThrasher Jun 4, 2024 • edited

Choose a reason for hiding this comment

eXpl0it3r Jun 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vittorioromeo Jun 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisThrasher Jun 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binary1248 commented Jun 4, 2024

Remove default empty state of `sf::Event` #2992

Remove default empty state of `sf::Event` #2992

ChrisThrasher commented May 11, 2024 •

edited

coveralls commented May 13, 2024 •

edited

L0laapk3 commented May 13, 2024 •

edited

vittorioromeo commented Jun 4, 2024 •

edited

ChrisThrasher commented Jun 4, 2024 •

edited

ChrisThrasher Jun 4, 2024 •

edited

eXpl0it3r Jun 11, 2024 •

edited

vittorioromeo Jun 11, 2024 •

edited

ChrisThrasher Jun 12, 2024 •

edited