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

Split display/window wrappers #64

Merged
merged 1 commit into from
Jan 11, 2023
Merged

Split display/window wrappers #64

merged 1 commit into from
Jan 11, 2023

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Jan 6, 2023

WIP implementation of #37. Currently compiles on Linux, and splits parts of Wayland code to not need duplication for multiple windows.

Seems best to start with this API change before moving on to #40.

@ids1024 ids1024 force-pushed the display-window-split branch 2 times, most recently from 0bebf35 to 268a4bd Compare January 6, 2023 23:10
@notgull
Copy link
Member

notgull commented Jan 7, 2023

Context is expected to be created once per connection, which brings up how often Surfaces are expected to be created. Should it be created once per surface, kept around for the lifetime of that surface, and dropped only when the surface is dropped? Or would it be perfectly cheap to create and drop Surfaces, like so:

match event {
    Event::RedrawRequested => {
        let surface = Surface::new(&context, my_window);
        /* .. */
        drop(surface);
    }
}

If the latter, we should move some of the allocations out of Surface into the Context. In either case, this should be documented.

src/lib.rs Outdated Show resolved Hide resolved
@ids1024
Copy link
Member Author

ids1024 commented Jan 7, 2023

I'm assuming one would keep the Surface and continue using it. So an allocations that are specific to the surface would be there.

A `Context` is created with a display handle, and a `Surface` is created
with a `&Context` and a window handle. Thus multiple windows can be
created from the same context without duplicating anything that can be
shared. This API is broadly similar to `wgpu` or `glutin`.

On Wayland, the `Context` contains the `EventQueue`, which is shared
between windows, and the `WlShm` global. On X11, `Context::new` checks
for the availability of XShm, and contains a bool representing that as
well as the `XCBConnection`. The shared context data is stored within
the window in an `Arc`.

On other platforms, the display isn't used and `Context` is empty. This
does however test that the display handle has the right type on those
platforms and fail otherwise. Previously the code didn't test that.

Closes #37.
@ids1024 ids1024 force-pushed the display-window-split branch from 268a4bd to 1290699 Compare January 7, 2023 05:37
@ids1024 ids1024 marked this pull request as ready for review January 7, 2023 05:37
@ids1024 ids1024 requested a review from john01dav as a code owner January 7, 2023 05:37
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

I'm wondering if it would be more idiomatic to make set_buffer take &Context as an argument. This would mean that we wouldn't need to have an Arc<Context> for Wayland and X11. This is less of a performance thing and more of an "is this more idiomatic?" thing.

LGTM otherwise

@ids1024
Copy link
Member Author

ids1024 commented Jan 9, 2023

Maybe. Though taking a &Context could be a bit more inconvenient after #40 since that will add more methods to Surface. And then you either have to pass it to all of them, or it's necessary to determine which methods might need something from the context (on some backends).

There could also be weird behavior if you passed a different context, though that probably isn't too much of an issue.

(Perhaps the ideal solution would be a lifetime bound on Surface if Rust had some kind of magical solution to make self-referential structs convenient, but that doesn't exist and makes lifetime bounds often awkward to deal with.)

@ids1024 ids1024 merged commit 125ad07 into master Jan 11, 2023
@ids1024 ids1024 deleted the display-window-split branch January 11, 2023 00:05
@daxpedda daxpedda mentioned this pull request Feb 28, 2023
@notgull notgull mentioned this pull request Jun 4, 2023
notgull added a commit that referenced this pull request Jun 4, 2023
* On MacOS, the contents scale is updated when set_buffer() is called, to adapt when the window is on a new screen (#68).
* **Breaking:** Split the `GraphicsContext` type into `Context` and `Surface` (#64).
* On Web, cache the document in the `Context` type (#66).
* **Breaking:** Introduce a new "owned buffer" for no-copy presentation (#65).
* Enable support for multi-threaded WASM (#77).
* Fix buffer resizing on X11 (#69).
* Add a set of functions for handling buffer damage (#99).
* Add a `fetch()` function for getting the window contents (#104).
* Bump MSRV to 1.64 (#81).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants