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

Stabilized window callbacks #551

Merged

Conversation

bohdloss
Copy link
Contributor

@bohdloss bohdloss commented Nov 6, 2023

Window callbacks were previously behind a feature gate because the implementation i wrote was inherently unsafe. This was because the user could move the window object out of the Box after creating the window, and this would break the new callback code that requires the window pointer to never change. It just occurred to me to wrap the Box<Window> inside a struct PWindow with its own Deref and DerefMut implementations, preventing the user from taking the window out of the wrapper. This effectively stabilizes the new callback implementation so i removed the "glfw-callbacks" feature from Cargo.toml. Let me know if there is any problem, even tho i tested it and it seems to work.

@bohdloss
Copy link
Contributor Author

bohdloss commented Nov 6, 2023

I also discovered a memory issue regarding the use of channels. Basically every kind of callback that uses polling will cause an allocation (window resizing events, mouse enter and exit events, etc...) and a subsequent deallocation. The solution i implemented was a custom channel type which i called GlfwSender / GlfwReceiver that internally uses a VecDeque in order to avoid frequent allocations. Does this sound reasonable? If it does i could commit it directly to this pull request to avoid having to merge 2 different PRs, if that is ok. Thank you for your time!

@bohdloss
Copy link
Contributor Author

bohdloss commented Nov 6, 2023

One last adjustment i made is that if the GlfwReceiver queue goes beyond a certain size, in order not to allocate too much memory for the VecDeque, it will fall back to normal mpsc channels. I initially configured the initial VecDeque capacity to be 16 and the maximum to be 256 (after which mpsc channels will be used until the queue is flushed again). These are of course very arbitrary values and i encourage to tweak them as it seems suitable after possibly merging. Again let me know if all this seems reasonable and if i should commit it. Thanks!

@bvssvni
Copy link
Member

bvssvni commented Nov 9, 2023

@bohdloss No problem merging separate PRs.

Merging.

@bvssvni bvssvni merged commit 91861f8 into PistonDevelopers:master Nov 9, 2023
@bvssvni
Copy link
Member

bvssvni commented Nov 9, 2023

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants