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

Migrate to glutin 0.25 #252

Merged
merged 1 commit into from
Nov 22, 2020
Merged

Conversation

jerry73204
Copy link
Contributor

Some variables/methods are renamed to reflect the change in glutin. It also fixes #249 to build with nightly compiler.

Copy link
Collaborator

@alvinhochun alvinhochun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, this patch doesn't look like it would work properly. Which platforms have you been testing it on?

src/window/gl_canvas.rs Outdated Show resolved Hide resolved
@alvinhochun
Copy link
Collaborator

Frankly, I don't believe that faking poll_events is the way forward. I have hopes that kiss3d should eventually discontinue the manual Window::render() loop model and completely replace it with kiss3d::window::State and Window::render_loop(), which seems to fit nicely with the new winit event loop. Also, Window::render_loop() is already a requirement when using on WASM. After transitioning to the new winit event loop, we might even be able to leverage the WASM support of winit and remove the extra web canvas implementation currently in kiss3d to greatly simplify the code.

Let's wait for @sebcrozet's comment.

@jerry73204
Copy link
Contributor Author

jerry73204 commented Oct 29, 2020

Sounds like a great change to me. Actually I made the change is simply due to #249, which breaks our project. Perhaps we could go in stages: Fix the issue first, and we can move on to the refactoring?

EDIT: grammar

@alvinhochun
Copy link
Collaborator

What I need to make sure is that it actually functions as expected. I may test on Windows (when I have the time to do so), but I don't have a macOS system to test with. The behaviour of run_return isn't really well-defined so someone will really need to check how it runs on a real system.

@jerry73204
Copy link
Contributor Author

I looked into the actual implementation in winit. It seems they guarantee the presence of the events MainEventsCleared -> RedrawEventsCleared in the order, and finally receive LoopDestroyed event if control flow is set to Exit. I think it's safe to exit when RedrawEventsCleared event arrives. Anyway, having a test would be necessary.

I don't have Windows and MacOSX. Suppose we call for testers?

Impls in winit:

@alvinhochun alvinhochun self-requested a review November 1, 2020 06:59
@sebcrozet
Copy link
Owner

Thank you for this PR!
I tried it on MacOS, Windows, and Linux, and it works properly on all three. Considering Kiss3d is basically unusable with any recent version of Rust, I think we should merge this PR, and then think of a possible redesign of the way we manage our render loop.

The solution suggested by @alvinhochun, to deprecate the Window::render in favor of Window::render_loop sounds like our best option right now. Though I find it unfortunate that we have to sacrifice the simplicity of Window::render.

@sebcrozet sebcrozet merged commit d39bbf5 into sebcrozet:master Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants