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

Exit interview #2 #889

Closed
makepaddev opened this issue May 30, 2019 · 15 comments
Closed

Exit interview #2 #889

makepaddev opened this issue May 30, 2019 · 15 comments
Labels
C - needs discussion Direction must be ironed out S - meta Project governance

Comments

@makepaddev
Copy link

makepaddev commented May 30, 2019

As i think winit is useful to the rust community here is my exit interview #2. Windows.

My last issue was just on mac/web, but since i'm redoing the platform layer on windows heres some feedback:.

WHY IS EVERYTHING THREADED. sorry but really. I just reduced thousands of lines of winit complexity crap to like 5 platform calls and a windows message loop. See, this is what you would expect.
https://github.com/makepad/makepad/blob/master/render/src/cx_windows.rs
This is about as complex as the windows platform layer should be. I've also reduced Cocoa to
this
https://github.com/makepad/makepad/blob/master/render/src/cx_cocoa.rs

Sorry but winit is just architected wrong, done wrong, everything is wrong. I can't use it, i think its bad for the Rust community to exist in its current form. Please reconsider everything. I emplore you, with live working example code on how i would rewrite winit from scratch for all platforms. Windows, mac, web and linux is coming up.

So. I'll make a more detailed post later, but i'm fixing it regardless. Please. Its bad. think about how you are doing things, its bad for Rust.

Goodluck.

@makepaddev
Copy link
Author

Also you have a C++ dependency that doesn't build on GNU windows. Come the F* on. Really.

@goddessfreya goddessfreya added C - needs discussion Direction must be ironed out S - meta Project governance labels May 30, 2019
@makepaddev
Copy link
Author

Reason i'm upset about a (needless) C++ dep on windows is it destroys the trivial-compile-anywhere-easily idea of Rust which it truly has from the compiler/stack even the winapi level. People put real hard effort in making this work as smoothly as it does. Only to be undone by a careless C++ dep here. So there.

@goddessfreya
Copy link
Contributor

A) Could you be a bit less aggressive?

Please be kind and courteous. There’s no need to be mean or rude.

Respect that people have differences of opinion and that every design or implementation choice carries a trade-off and numerous costs. There is seldom a right answer.

Please keep unstructured critique to a minimum. If you have solid ideas you want to experiment with, make a fork and see how it works.

-- https://www.rust-lang.org/policies/code-of-conduct

B) We try out best to support multi threading because that's one of Rust's strengths. Not everything is multi threaded however. Examples: EventLoop, macos's Window possibly (see #873)

C) Winit is as complex as it is for two reasons:
1- partially because of bad design (which we are always willing and trying to improve, see the eventsloop-v2 branch); but mostly
2- because we have to handle more than one use case. I suspect your 300 line windows implementation does not provide all the functionality that winit provides and does not cover all use cases.

Nevertheless, if those 300 lines are all you need for your use case, go for it! Maybe even write Winit-lite, or better yet, Winit-not (shortened as Winot)!

D) I wasn't, nor do I think anyone else was aware that this - so far unnamed - dep was causing compile issues. Here's how you can help:
1- Add to our CI suit some builds that use MinGW so we can spot these kind of issues in the future before merging PRs.
2- Fix the compile issue.

@makepaddev
Copy link
Author

Sorry i've been asked to give my feedback instead of just ignoring it and moving on (via twitter).

@makepaddev
Copy link
Author

Since apparently lots of people are having problems but not giving feedback leaving the project nothing to go on.

@makepaddev
Copy link
Author

Also i've long since realised i can't 'fix' winit with pull requests since i'd really rewrite the whole thing from scratch. But its also 'kinda' hurting Rust as an ecosystem by being architected so poorly.

@makepaddev
Copy link
Author

And yes, i understand the 'more than one usecase' thing, but that doesn't mean the primary Rust window-abstraction library is almost unusable for anyone standing up a real product on it, as i am, and have been running into massive unfixable issues over the last few months.

@makepaddev
Copy link
Author

So sorry its nothing personal, i'm not angry at any one person, or you, but i am angry that this is the state of multiplatform windowing on Rust. Its garbage. Sorry, and really consider the architecture. I can't be more forceful about it than i am. Working code in links. And yes, my 300 loc windows thing is just the beginning, but it will work just fine.

@jdm
Copy link
Contributor

jdm commented May 30, 2019

Just because you feel strongly that winit does not serve your needs does not mean it is acceptable to say things like "it's garbage". Feedback is one thing; insults are unwarranted.

@makepaddev
Copy link
Author

And the argument 'you dont have our usecase' yea ok, fine, but if you wanna be a fullscreen GL thing whats all the windowing for then eh :) The cocoa winit thing missed core features like 'being able to reliably start up more than 4 out of 5 times'. Clipboard, IME support. The windows thing didnt' even get the core message loop concept right. Sorry but. yea. Its harmful to the Rust ecosystem.

@makepaddev
Copy link
Author

So yea sure it may be hurtful, it may be harsh. but you know what is harsh, releasing a software product on top of Winit and expecting to get paid for it by real users. So yeah. you get harsh feedback. Its not personal right. But winit? as a project? Yeah its total garbage. Period.

@makepaddev
Copy link
Author

Anyway, flamewar. I'm here to help i'm here to point in the way i think things should be. Thats why i bother to respond. :)

@rust-windowing rust-windowing locked as too heated and limited conversation to collaborators May 30, 2019
@goddessfreya
Copy link
Contributor

goddessfreya commented May 30, 2019

Also i've long since realised i can't 'fix' winit with pull requests since i'd really rewrite the whole thing from scratch. But its also 'kinda' hurting Rust as an ecosystem by being architected so poorly.

I must disagree. Major issues have been fixed w/complete rewrites via PRs, just see all the ones accepted for eventloop-v2: https://github.com/rust-windowing/winit/milestone/2?closed=1

However, before any such major changes can be taken, a "design doc" outlining the architecture changes should be drafted and consensus must be reached wrt it.

See:
#837 (needs to be started on)
#459 (achieved for all desktop platforms)
#804 (windows only currently)

[..] The cocoa winit thing missed core features like 'being able to reliably start up more than 4 out of 5 times'. Clipboard, IME support. The windows thing didnt' even get the core message loop concept right. [..]

If you submit PRs solving these issues we would eternally be gratefully. You are right, these features are absolutely useful, however, we currently lack the manpower to get them done. We currently have a small hand-full of maintainers, and just lack the time.

It is a very sad state-of-affairs for such an important building block of the Rust graphics ecosystem to be maintained by less people then I have fingers, so I encourage you - and others - to submit solutions to problems as you encounter them.

See #830.

So yea sure it may be hurtful, it may be harsh. but you know what is harsh, releasing a software product on top of Winit and expecting to get paid for it by real users. So yeah. you get harsh feedback. Its not personal right. But winit? as a project? Yeah its total garbage. Period.

You're getting paid, not us. All of the current maintainers volunteer their time for free. Anyways, if you're willing to pay us to resolve certain issues, file them then place a bounty on them. Hopefully that will attract someone to solve it when we start our contributor push, as mentioned in #830, although no guarantees they'll actually get done

Anyways, I'm locking this down for 24 hours. Consider this as a warning that this behavior is not tolerated.

EDIT: s/figer/finger/

@Osspial
Copy link
Contributor

Osspial commented May 30, 2019

Ya'know, I've been here.

The first time I really took a deep dive into Winit's codebase, I came to a similar conclusion to what you did! I distinctly remembering being angry about Winit using threads for the Windows backend. It is indeed seemingly unnecessary complexity that simply serves to add onto Winit's bugginess.

So, programming-wise, I did exactly what you're doing right now! I went and implemented my own windowing library as an attempt to trim away the complexity added by Winit which, seemingly, doesn't need to be there. You can go check that out here.

But... well, that project's been abandoned. I'm sitting here as the Windows maintainer of Winit, and not as the creator of a glorious replacement that got rid of all the unnecessary shit supposedly weighing down its codebase. Now, why do you think that is?

Turns out, all that cruft isn't just there for show.

WHY IS EVERYTHING THREADED. sorry but really. I just reduced thousands of lines of winit complexity crap to like 5 platform calls and a windows message loop. See, this is what you would expect.
https://github.com/makepad/makepad/blob/master/render/src/cx_windows.rs

I agree! That is what you'd expect. But, when you're wrapping around a complex system, you need to make design tradeoffs to work around the quirks of the underlying system. One of the sneakiest limitations of the Windows API is that resizing a window freezes externally-driven event loops. If you're creating a windowing library that's used for more than just games (as you seem to want), that's a complete non-starter: users expect windows to resize without completely breaking the application. The solution for that particular problem is to make sure the event loop isn't externally driven; Winit's original solution was to shunt the event loop to a background thread, hiding the freezing by making sure it didn't happen on the application's main thread. Obviously that solution comes with further tradeoffs, and in the end I'd agree that they aren't worth it. That's why we've re-designed the entire API to get rid of the thread without exposing an API that freezes the application's code. You may have heard the redesign - I only mentioned it extensively the last time you posted about this.

This is about as complex as the windows platform layer should be.

You forget that sometimes, an application needs to have more than one window.

Also you have a C++ dependency that doesn't build on GNU windows. Come the F* on. Really.

Pretty sure that's because of a misconfigured GNU environment on your end, not due to us. windows-GNU takes an astonishing amount of work to set up on Windows - my guess is that you don't have one of the many MinGW systems installed and in your path. It's necessary if you want to link to any non-Rust libraries.

(although I did find out that I'd accidentally left backtrace in Cargo.toml on eventloop-2.0, even though it's not used in the code. Thanks for the reminder to take it out :))

The cocoa winit thing missed core features like 'being able to reliably start up more than 4 out of 5 times'.

Please read my response from the previous thread.


So... long story short, I'd agree that the Winit on master is in a sorry state. But, the situation has changed since then. If you're going to complain about the state of Winit, please be aware that we're aware, and that we've gone a long way towards fixing it. It's available here, if you'd like to take some time away from replacing us to look at it. Just... like @zegentzy said, try to be respectful when you're talking to people.

@rust-windowing rust-windowing unlocked this conversation May 30, 2019
@makepaddev
Copy link
Author

Hi people, i want to apologise for my behaviour. I had just spent a long day cutting complexity and was a bit upset. However its not excusable as opensource is a matter of free time / love to create something and i just made that a bit more crap for you. And for that i am sorry. I use Winit as a reference code base almost every day and its been massively valuable to me so thank you all for making that happen.
As for the new messageloop 2.0, the code indeed does look a lot better, and after thinking on it it may even be the best approach if you can 'sync' the background thread with the foreground for important events like 'resize' so it doesn't start to lag.
I don't see how it could be unified with wasm32-unknown-unknown but maybe it doesn't have to since that requires a completely different codepath anyway (as i have with my 'main' macro). All it needs is a bit of JS to match mouse events/keyboard events and that works as well.
It seems like winit is on the right path, and i wish the project the best to be a corner stone of Rust frontend development.
Messageloop 2.0 also gave me some ideas, as to write the eventloop as something that never returns, but making the 'all events done' state a separate event inside. That way i will attempt to do the UI without threads, by 'faking' the 'idle' event with a timer when resizing so i don't need to own the message pump itself to do the renderloop, possibly WM_IDLE on windows will do as well. Arguably that is a less general solution than the threads, but it will be simpler. If that somehow fails who knows i might be back on winit+msgloop 2.0.

Have a nice day, and happy coding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - needs discussion Direction must be ironed out S - meta Project governance
Development

No branches or pull requests

4 participants