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

Add method for updating the buffer only #174

Closed
wants to merge 2 commits into from
Closed

Add method for updating the buffer only #174

wants to merge 2 commits into from

Conversation

christoph-heiss
Copy link

@christoph-heiss christoph-heiss commented Apr 27, 2020

Hi,
I basically needed a way to update the framebuffer independently of input due to blocking and expense of updating the input state.

I had this branch laying around for some time now, I rebased & updated the changes so the branch can be merged, before it completely code-rots. Although very specialized, I'd like to get this upstream.

Tested only on Linux/X11, but all the code is basically a copy of update_with_buffer_stride with any input updates ripped out.

I'm open for suggestions and be willing to further work on this if needed in order to get this merged.

@christoph-heiss christoph-heiss changed the title Add method for updating the buffer only. Add method for updating the buffer only Apr 28, 2020
@nyovaya
Copy link
Contributor

nyovaya commented May 20, 2020

@christoph-heiss I think it might be senseful then to remove the update_buffer_with_stride function. So that we only have a function which updates input and one which updates the framebuffer(in the public API).

@emoon
Copy link
Owner

emoon commented May 21, 2020

I agree with @nifker here. That would make it a bit cleaner

After this change, calling `.update()` will only update the input state
of the window.
To update the framebuffer, `.update_buffer()` has to be called separatly.
@christoph-heiss
Copy link
Author

As suggested, I've removed the .update_buffer_with_stride() completely.
I then also renamed the remaining method to simply .update_buffer() to better represent the new API.

The second commit just adapts all examples to the new Window API.

As always, only tested on Linux/X11 and RFC'ing on this.

@nyovaya
Copy link
Contributor

nyovaya commented May 22, 2020

@christoph-heiss Seems fine to me. But Wayland is struggling a bit here - Ill take a look at it.

@nyovaya
Copy link
Contributor

nyovaya commented May 22, 2020

@christoph-heiss Ok we are using single files on Wayland for a framebuffer so it would be nice if you include a note for Wayland that "when the buffer is too often updated without using update() the application will crash because too many files have been opened".
I currently think of returning an error if there were more than 10 continuous buffer updates without any update() call.

@nyovaya
Copy link
Contributor

nyovaya commented May 22, 2020

I think there is no compositor which holds 20 framebuffers at the same time.
I would add this in line '1104' in 'wayland.rs':
"assert!(self.display.buf_pool.pool.len() < 20, "Leaking file descriptors");"

@christoph-heiss
Copy link
Author

@nifker TBH, that sounds kind of like a hack to me. I'd be willing to work on a proper solution for this and can set me up a wayland-environment for this, I guess.

This sounds like the compositor needs to explicitly "consume" the frames in-flight we write out to the fd (don't really know the proper wayland-way words, never worked with it before), and this behaviour must be kind of hidden(?) somewhere in .update().

I'll see what I can do when I get around that, so the PR can stay open as-is for now.

@nyovaya
Copy link
Contributor

nyovaya commented May 22, 2020

Not sure if I can figure something better out, as calling update() will be mandatory anyway, so the developer just needs to make sure to call it once in a while.

@emoon
Copy link
Owner

emoon commented May 15, 2022

Closing due to no progress, if this is still of interest we can re-open

@emoon emoon closed this May 15, 2022
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.

3 participants