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

Alternative version of callback-taking functions for making API bindings #1004

Open
danielkeller opened this issue Dec 27, 2021 · 16 comments
Open

Comments

@danielkeller
Copy link
Contributor

I'm working out how to wrap egui's API for my scripting language. Something that would make this a lot easier is if there was an alternative API to the functions that take callbacks (like SidePanel::show, Window::show, and Ui::horizontal) which could be written in straight-line code. For example, an object that exposes a &mut UI to draw widgets, and has a finish method which drops it.

It looks like I can emulate this with some pairs of Ui::new and Ui::allocate_rect, but I'll have to rewrite all the useful stuff those functions do.

@coderedart
Copy link
Contributor

I might be wrong, but i think the point of closures is that they will run only if necessary (ui is shown).

With a straight sequence style of begin and end functions, we will have to throw around if shown else not shown kind of code. Or is there a more idiomatic code style?

@emilk
Copy link
Owner

emilk commented Dec 27, 2021

egui passes closures to functions to call, like this:

ui.collapsing_header("Foo", |ui| {
    // …
});

These aren't really "callbacks" in the classical sense since they are called immediately (or never).

The reason for them is that the parent Ui must run some setup-code and then some tear-down code after the child has been layed out. In the code above there are two different Ui both with the name ui (parent/child), but since we only ever care about the innermost, this is a good use of shadowing.


One alternative could be this (used e.g. by Dear ImGui and Nuklear):

if ui.collapsing_header_begin("Foo") {
    // …
}
ui.collapsing_header_end();

This has two downsides:

A) it is way to easy to put the end call in the wrong place (or forget it completely), leading to a lot of subtle bugs that only show up a t runtime.
B) it requires the library (egui) to keep a manual stack of what is going on

We could of course consider adding support for a lower lever API like this, but due to B) above it would require a huge rewrite and redesign of egui.


Another alternative us using linear types (types you must manually close):

if let Some(child) = parent.collapsing_header("Foo") {
    // …
    child.end(parent); // must call, or it won't compile
}

Linear types aren't supported by Rust (though you can approximate them with hacks that yields linker errors if you fail to close them), but it also creates a lot of child/parent bindings, which can be confusing.


If Rust had something like Pythons with statements we could do:

with ui = ui.collapsing_header("Foo") {
   // …
}

and have the compiler insert the end call automatically. But Rust doesn't support anything like this.

@danielkeller
Copy link
Contributor Author

Thanks for the explanation! What about something like RAII objects? This seems to be the pattern the standard library uses to run tear-down code, for example unlocking mutexes.

@parasyte
Copy link
Contributor

It might be ok to impl Drop on structs returned by parents. The only catch is that Rust doesn't guarantee destructors are always called. This is why e.g. crossbeam::scope takes a callable. For any cases where not calling the destructor would result in unsoundness, this is currently the best pattern offered by the language.

@emilk
Copy link
Owner

emilk commented Dec 30, 2021

A RAII approach would also require a Ui to keep a reference to its parent Ui, leading to a lot of hassle with lifetimes.

I would be more interested in investigating a more low-level C-like API (with begin/end) and then build the current Ui interface on top of that as a convenience. But that's a lot of work. It also is not obvious how one would handle third-party containers.

@emilk
Copy link
Owner

emilk commented Dec 31, 2021

Another downside of the current closure approach is code bloat and long compile times (due to excessive monomorphization)

@lunixbochs
Copy link
Contributor

+1, I want to bind egui to python but the callback approach is much worse than something I can wrap in a context manager

I'm imagining this to prototype it:

@ui.collapsing_header
def cb(ui):
    ...

but I'd really like to be able to do something like this (which I confirmed properly nests scope in Python without persistently shadowing the outer ui variable)

with ui.collapsing_header() as ui:
    ...

My vote is to try this with Drop:

if let Some(ui) = ui.collapsing_header() {
    ...
}

I looked into Drop a bit and it seems like it should be stable enough for RAII in most cases: https://aloso.github.io/2021/03/18/raii-guards.html#when-drop-isnt-called

I think Box::leak doesn't apply unless you're returning a boxed ui object. Same goes for reference counting cycles.

@lunixbochs
Copy link
Contributor

lunixbochs commented Mar 10, 2022

I did a survey of how egui tends uses the inner responses internally after calling add_contents. It mostly accesses the rect and propagates the closure's return value up.

@schungx
Copy link

schungx commented Mar 13, 2022

I'm working out how to wrap egui's API for my scripting language. Something that would make this a lot easier is if there was an alternative API to the functions that take callbacks (like SidePanel::show, Window::show, and Ui::horizontal) which could be written in straight-line code. For example, an object that exposes a &mut UI to draw widgets, and has a finish method which drops it.

Yes, a fluent API is great for programmers, but not friendly to automation, which is what a scripting system ultimately is.

There should be a low-level, imperative, step-by-step API for machine-controlled environments.

@lunixbochs
Copy link
Contributor

lunixbochs commented Mar 15, 2022

This is incomplete, but I'm working on a concept here:

  1. Playing around with a Context Manager API: https://gist.github.com/rust-play/8667abe111edeb90bca9a44687ecdca4
  2. Working on structurally wiring some of the ui api up to it: https://gist.github.com/rust-play/c618172551bebd4f93d929f870d4baa4

Basically, a method like ui.horizontal() would return an object conforming to the UiContextManager trait, which will need to run before and after the user's inner UI code. To make a context manager work, we really just need some state and an exit() method (or a Drop impl). Nested context managers will work fine, you just chain the exit() methods.

Children don't need a reference to their parent, the context manager can be a short lived object that has a reference to the parent, the child, the mutable Result object, and its internal state necessary for exit().

The ui.* api can be moved to a trait as demonstrated, so you can call ui methods like ui.horizontal() directly on the returned context manager.

This can be made compatible with the existing API:

impl Ui {
    fn horizontal(add_contents) {
        if let Some(ctx) = self.horizontal_ctx() {
            inner = add_contents(ctx.ui);
            ctx.exit(); // optional, drop could also call this
        }
        inner // also copy the Response out of ctx before getting here if we want an InnerResponse
    }
    fn horizontal_ctx() -> Option<ContextManager> {
    }
}

You could also provide this as separate API traits / structs, used as a backend for old callback API, but with similar method names.

@lunixbochs
Copy link
Contributor

lunixbochs commented Mar 16, 2022

This approach seems somewhat clean for defining the cleanup function at call sites:
https://gist.github.com/rust-play/5dbf90f3c22fcec9ce0de00cb87f4273

If necessary the Context object could be either dynamic or templated, allowing each specific call site to stash arbitrary data.

@coderedart
Copy link
Contributor

coderedart commented Mar 16, 2022

If there is going to be a partial low level rewrite of egui api, then we might as well check if the new api will be suitable to write bindings for at the very least in wasm/deno_core/pyo3 or rustpython/mlua. If the new api is fine for one of these and not for others, it would be really sad. Ideally egui will have a simpler C based abi / ffi compatible as that will be the most compatible with all languages. (including rust dylibs)

@lunixbochs
Copy link
Contributor

lunixbochs commented Mar 16, 2022

we might as well check if the new api will be suitable to write bindings

Yes, I'm specifically doing this for PyO3. (Technically you could probably bind to the previous API, but it would require a really awkward rust -> language -> rust -> language stack to satisfy the egui callback API.)


I've prototyped a new context manager style here: https://gist.github.com/d830916a42f650eae3a590e24b0d1504

It allows you to write widgets something like this, with both the setup and teardown code in the same function:

fn horizontal(&mut self, render: bool) -> Option<ContextManager> {
    if !render { return None; }
    let child = Ui{id: self.id + 1, parent: self.id, data: 0};
    let local = 1;
    self.cm(child, move |parent, child| {
        println!("cm exit parent={:?} child={:?} local={}", parent, child, local);
        child.data += 1;
        parent.data += 1;
    })
}

fn cm(&mut self, child: Ui, f: impl FnOnce(&mut Ui, &mut Ui) + 'static) -> Option<ContextManager> {
    Some(ContextManager::new(f, self, child))
}

It can be polyfilled to the old callback API style like this:

fn horizontal_cb<R>(&mut self, f: impl FnOnce(&mut Self) -> R, render: bool) -> Option<R> {
    match self.horizontal(render) {
        Some(mut cm) => Some(f(&mut cm.child)),
        None => None,
    }
}

(You'd combine this with my Ui trait e.g. shown here #1004 (comment) so both the ContextManager and Ui objects can have .horizontal() called on them without redundant implementations)

@emilk thoughts?

@lunixbochs
Copy link
Contributor

lunixbochs commented Mar 16, 2022

I started a port here: https://github.com/talonvoice/egui/tree/no-callback

I ported everything in ui.rs to my context manager approach, with a _ctx method suffix. I modified all of the add_contents methods to use the equivalent _ctx method internally. At a glance, the egui demo app appears to completely work and perform the same.

Here's an example of one of the method pairs: https://github.com/talonvoice/egui/blob/no-callback/egui/src/ui.rs#L892-L928

I have two notes:

  • I'm managing the lifetime of Response objects using an Option type. The add_contents compatibility methods end up calling unwrap() on the response (at a point in time where I know the Response has been set), because InnerResponse isn't optional.
  • My ContextManager assumes only one child UI object, and that assumption broke for columns(), so columns is not properly wrapped for now.

@emilk emilk changed the title Alternative version of callback-taking functions Alternative version of callback-taking functions for making API bindings Apr 3, 2022
@Starwort
Copy link

Starwort commented Jun 8, 2022

Another alternative us using linear types (types you must manually close):

if let Some(child) = parent.collapsing_header("Foo") {
    // …
    child.end(parent); // must call, or it won't compile
}

Linear types aren't supported by Rust (though you can approximate them with hacks that yields linker errors if you fail to close them), but it also creates a lot of child/parent bindings, which can be confusing.

If you implement Drop for child then this could work: Playground

@Danvil
Copy link

Danvil commented Oct 19, 2023

I am automatically generating my GUI and it would greatly simplify my life if there would be a simple "low-level" API like grid_begin(..) / grid_end(...). The closure style makes it hard to show elements on the GUI "as they come". Instead I basically have to first collect all items of a group before I can create the parent element (like Grid) as the closure needs all elements when/if the show function is called.

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

No branches or pull requests

8 participants