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

What are you doing with Event? #2990

Closed
3 tasks done
kelbon opened this issue May 9, 2024 · 6 comments
Closed
3 tasks done

What are you doing with Event? #2990

kelbon opened this issue May 9, 2024 · 6 comments
Labels

Comments

@kelbon
Copy link

kelbon commented May 9, 2024

Prerequisite Checklist

Describe your issue here

SFML was good until
#2766

Firstly. No one (literaly no one) had problems with some 'type safety', no one asks to make such great changes which is fully broke all existing code ever.

Problems:
-1. it "solves" problems which never existed
0. it breaks all code
2. It is unusable (if if if if if) and no way to make clear code with switch and field access. Code will be less readable (both for me and compiler) and less maintanable
3. it is not efficient.

Author says, that he does not want to add .visit because it will break code later with adding events. But he break all existing code now and it is unusable. I think real reason why he does not add .visit is because Rust does not have .visit and it is impossible to implement

REMOVE RUST FROM MY C++

Your Environment

any environment

Steps to reproduce

  1. have some code with using SFML

Expected behavior

Library is usefull

Actual behavior

Library unusable

@kelbon kelbon added the bug label May 9, 2024
@Bambo-Borris
Copy link
Contributor

The API change is still in the works, it isn't its final form. There was also ample time to voice concerns and discuss it during the PR of #2766. The changes do solve problems of users being able to misuse the sf::Event union from 2.x days, many users enter the SFML Discord & forums with bugs and issues that trace back to misusing the sf::Event union. It is perfectly usable, but if you prefer the switch case there was an argument put forward by people about this during the PR, and since, and I've seen maintainers are more than open to discussion about this. Also master is the in development branch of SFML 3.0.0, which is a major version update that features breaking API changes. Which under sem ver is the only time SFML can undertake such changes. If you've been using master, you've been utilising an in development version which is still subject to change, expect breaking changes to come if you choose to use that branch.

For what it's worth, the attitude you've put forward in this issue isn't one that helps drive reasonable discussion about things like this. The points raised aren't backed up by any evidence of real concerns and problems either in performance (with data) or ergonomics (showing code examples). The people contributing to, or maintaining SFML are doing it voluntarily, and I think your tone undermines the value of their work. You don't have to argree with the changes made, but you can put forward your ideas and problems in a less aggressive way.

@sabudilovskiy
Copy link

["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.eXpl0it3r]

In the original pull request, they specifically decided to merge to master to see the reaction of people. Although I don't quite agree with the tone of the discussion set by Kelbon, but I want to note that he notices the disadvantages of the new API: first, the standard use of getIf produces scopes that are extremely unreadable. And besides, these are just unnecessary checks before calling the function and after calling the function.

Not adding a visit is a monstrous mistake, because in such a context, it is possible not to lose performance only with the help of a matcher. ChrisThrasher's initial remark that changing event types will break the assembly is simply absurd, because if a person intentionally does not add an overload with auto& in the visitor, then he usually expects an exact match of types and wants to get compilation errors if the event types have changed. Isn't this true type safety?

For me, the best solution is to keep the current interface unchanged and add few new functions: visit_event, getIf, is.

@Bambo-Borris
Copy link
Contributor

["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.eXpl0it3r]

In the original pull request, they specifically decided to merge to master to see the reaction of people. Although I don't quite agree with the tone of the discussion set by Kelbon, but I want to note that he notices the disadvantages of the new API: first, the standard use of getIf produces scopes that are extremely unreadable. And besides, these are just unnecessary checks before calling the function and after calling the function.

Not adding a visit is a monstrous mistake, because in such a context, it is possible not to lose performance only with the help of a matcher. ChrisThrasher's initial remark that changing event types will break the assembly is simply absurd, because if a person intentionally does not add an overload with auto& in the visitor, then he usually expects an exact match of types and wants to get compilation errors if the event types have changed. Isn't this true type safety?

For me, the best solution is to keep the current interface unchanged and add few new functions: visit_event, getIf, is.

I was one of the supporters of retaining the ability to switch on event types, as it was a style I much preferred for handling events. I'm hoping to see what things the community suggests to improve the ergonomics of working with the new event API. If you have more deep example of how you'd like to work with the API I'd love to see some pseudo code samples!

@kelbon
Copy link
Author

kelbon commented May 9, 2024

["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.eXpl0it3r]
In the original pull request, they specifically decided to merge to master to see the reaction of people. Although I don't quite agree with the tone of the discussion set by Kelbon, but I want to note that he notices the disadvantages of the new API: first, the standard use of getIf produces scopes that are extremely unreadable. And besides, these are just unnecessary checks before calling the function and after calling the function.
Not adding a visit is a monstrous mistake, because in such a context, it is possible not to lose performance only with the help of a matcher. ChrisThrasher's initial remark that changing event types will break the assembly is simply absurd, because if a person intentionally does not add an overload with auto& in the visitor, then he usually expects an exact match of types and wants to get compilation errors if the event types have changed. Isn't this true type safety?
For me, the best solution is to keep the current interface unchanged and add few new functions: visit_event, getIf, is.

I was one of the supporters of retaining the ability to switch on event types, as it was a style I much preferred for handling events. I'm hoping to see what things the community suggests to improve the ergonomics of working with the new event API. If you have more deep example of how you'd like to work with the API I'd love to see some pseudo code samples!

I'd love to see some pseudo code samples!

First example:
as now, unchanged

New api example (As :@sabudilovskiy suggests):

overloaded matcher{
[] (Type1&) {
  //
},
[] (Type2&) {
  // 
};
};
sf::visit_event(event, matcher);
// or
if (Type1* t = event.getIf<Type1>)
  use(*t);

@ChrisThrasher
Copy link
Member

ChrisThrasher commented May 9, 2024

SFML 3 is still unreleased and under development. If you'd like to make API recommendations, we welcome that in a new issue where you can use more respectful language. If you would like to complain and call me names then you may do that elsewhere not in this repo.

@vittorioromeo
Copy link
Member

vittorioromeo commented May 11, 2024

Alright, I'll bite.

SFML was good until #2766

Correct, now it's very good.

Firstly. No one (literaly no one) had problems with some 'type safety'

Source: trust me bro. I have made mistakes with SFML 2.x events before that the compiler would have prevented with SFML 3.x, and I'm confident others have as well.

no one asks to make such great changes which is fully broke all existing code ever.

SFML 3.x is a new major release, breaking changes due to API improvements are expected. SFML 2.x is still available if you cannot afford to adapt your codebase.

By the way, adapting the new event API in my commercial SFML game "Open Hexagon" took less than 10 minutes and my code was more convoluted than it should have been. I suspect adapting simpler codebases will be even quicker and it's a simple mechanical effort that can actually reveal long-standing bugs.

Problems: -1. it "solves" problems which never existed 0. it breaks all code

You are repeating yourself.

  1. It is unusable (if if if if if) and no way to make clear code with switch and field access.

An if-else chain is usable, as proven by my existing project. It's also less error-prone than a switch as you cannot forget to add 'break', which is a common mistake.

Code will be less readable (both for me and compiler) and less maintanable

Subjective, and I disagree.

  1. it is not efficient.

It is. Even if it takes a few more cycles than a switch, it doesn't matter because event handling code overhead is insignificant compared to the rest of your application/game.

Author says, that he does not want to add .visit because it will break code later with adding events.

We can consider adding a visitation API in the future, but it's really not that much better than an if-else chain for events. There's additional syntactical overhead due to the creation of a visitor, even if you use the lambda-based pattern, which will require captures all over the place.

Still, not off the table.

But he break all existing code now and it is unusable.

Again, SFML 3.x is a breaking major release.

I think real reason why he does not add .visit is because Rust does not have .visit and it is impossible to implement

Every single part of this sentence is completely incorrect.

Rust has nothing to do with the changes to the event API. Rust has built-in pattern matching which is basically visitation, but much better. Visitation is not impossible to implement neither in Rust nor in C++.

REMOVE RUST FROM MY C++

Rust is not in your C++. If you want to complain about variants and optional being parts of the Standard, feel free to write an ISO paper and direct it to WG21. I'm sure they will love your technical argumentation and polite language.

Expected behavior

Library is usefull

Actual behavior

Library unusable

Skill issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants