Discussion: the road to 0.2

[Gekkio]

Just like we had #167 for 0.1, I'd like to mention some of the changes planned for 0.2 here. There's quite a lot of breaking changes in 0.2, but there's a common theme behind them: minimizing lifetime complexity caused by borrowing too early.

Widget builder APIs

Many widget builder APIs that have been added or updated, and in the new versions a &Ui reference is only needed during the final build call. Also, if the widget uses a single mutable reference (for example, to edit something), that reference is accepted in the build call, not in the new call.

The reason for this is simple: the builders are supposed to be mostly simple data, and the more complicated stuff happens only during the build call. Let's take the progress bar as a simple example:

0.1:

ui.progress_bar(0.6)
    .size([100.0, 12.0])
    .overlay_text(im_str!("Progress!"))
    .build();

In the 0.1 version, calling ui.progress_bar borrows the Ui, which causes a lot of complexity (all the PhantomData and lifetimes in the structs), and if we ever decide that we need to use mutable borrows, this makes many reasonable use cases completely impossible because we borrow too early.

0.2:

ProgressBar::new(0.6)
    .size([100.0, 12.0])
    .overlay_text(im_str!("Progress!"))
    .build(&ui);

In the 0.2 version, Ui is borrowed only very briefly when the widget is built, which is much closer to the upstream dear imgui API. If you don't call build, you also have a copiable/clonable/debuggable builder that is simple data and has no relation to the Ui struct.

Alternatives:

If we fully accept the fact that builders in this case are really just a way to handle default arguments, why not make ProgressBar an "optional argument struct" that just carries all the arguments that can be left to default values:

// Using defaults
ui.progress_bar(0.6, ProgressBar::default());

// Using custom arguments
ui.progress_bar(
    0.6,
    ProgressBar::default()
        .size([100.0, 12.0])
        .overlay_text(im_str!("Progress!")),
);

Personally I prefer at the moment the 0.2 approach, but this alternative is also perfectly valid.

Maybe you want all API calls to be accessible via ui.xxx? We could support this:

ui.progress_bar(0.6)
    .size([100.0, 12.0])
    .overlay_text(im_str!("Progress!"))
    .build(&ui);

However, this doesn't really make much sense because the ui.progress_bar call doesn't need or use the Ui reference, and you still need to pass it again to build.

How about having the function in the Ui type, but as a function that doesn't need the reference:

Ui::progress_bar(0.6)
    .size([100.0, 12.0])
    .overlay_text(im_str!("Progress!"))
    .build(&ui);

This is doable, but to me seems of little value...the main difference between ProgressBar::new and Ui::progress_bar is now in the imports: in the former case you need to import all the widgets you use (or access them explicitly with imgui:: prefix), while in the latter case everything is hidden behind one struct: Ui. In my opinion, the rustdoc documentation is much better in the former case, because you can actually see each widget builder as a separate thing instead of scrolling 100 pages of functions that Ui supports.

[Gekkio]

Stack tokens for push/pop, begin/end pairs

In 0.1, I started moving these kind of "paired API calls" towards stack tokens: structs that are returned from the push/begin call. In 0.1 they automatically call the corresponding pop/end function when they are dropped.

However, it turns out this wasn't a very good idea, because dear imgui really likes to assert things and abort the process. If we end up in any kind of panic situation (could be a simple problem in the user code), all the stack tokens are dropped during unwinding and it's very much possible that dear imgui decides to abort the process. The result: no stack trace of the actual problem and just a simple assert failure pointing to the C++ source.

In 0.2, I've basically reversed the role of the stack tokens: they must be ended manually, and the automatic Drop panics (and possibly aborts with a stack trace) if you forget to do this. This is closer to the original API, but in my opinion is better than relying on the asserts since it makes tracing the problem easier by showing a Rust backtrace in many cases.

Let's take a look at a simple case: pushing an ID to the ID stack, but forgetting to pop it.

0.0.x:

ui.push_id("lol");

imguitest: third-party/cimgui/imgui/imgui.cpp:4213: void CheckStacksSize(ImGuiWindow*, bool): Assertion `*p_backup == current && "PushID/PopID or TreeNode/TreePop Mismatch!"' failed.
[1]    30606 abort (core dumped)  cargo run

0.2.x:

ui.push_id("lol");

Compile-time warning:

warning: unused `imgui::stacks::IdStackToken` that must be used
  --> src/main.rs:12:5
   |
12 |     ui.push_id("lol");
   |     ^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unused_must_use)] on by default

warning: unused return value of `imgui::stacks::<impl imgui::Ui<'ui>>::push_id` that must be used
  --> src/main.rs:12:5
   |
12 |     ui.push_id("lol");
   |     ^^^^^^^^^^^^^^^^^^

Runtime error:

thread 'main' panicked at 'A IdStackToken was leaked. Did you call .pop()?', /home/joonas/projects/gekkio/imgui-rs/src/stacks.rs:420:13
stack backtrace:
........(omitted for simplicity)
   6: <imgui::stacks::IdStackToken as core::ops::drop::Drop>::drop
             at /home/joonas/projects/gekkio/imgui-rs/src/stacks.rs:420
   7: core::ptr::real_drop_in_place
             at /rustc/a53f9df32fbb0b5f4382caaad8f1a46f36ea887c/src/libcore/ptr.rs:195
   8: imguitest::main
             at src/main.rs:13
........(omitted for simplicity)

Note how imguitest::main src/main.rs:13 pinpoints the scope where the token is leaked.

This is how to fix the problem in 0.2:

let id = ui.push_id("lol");
// do stuff
id.pop(&ui);

Closures vs paired calls

In 0.1 I started deprecating some of the closure versions of these APIs, but this will be reverted. Using closures is often simpler than fiddling with tokens. However, there's one big downside to closures: behind the scenes Ui needs to borrowed only to call push/begin and pop/end, but in the closure API the borrow lasts for the entire time. I have a gut feeling we'll have to move towards mutable Ui references in the near future, and closures don't work nicely in that case.

For example, let's say we change the API to always borrow Ui mutably when the call mutates something. With the closure API this simple use case would no longer compile:

ui.with_id("lol", || {
  ui.text("foo"); // error, ui already mutably borrowed by ui.with_id
});

With the push/pop API it would still work:

let id = ui.push_id("lol"); // temporary mutable borrow
ui.text("foo"); // temporary mutable borrow
id.pop(&mut ui); // temporary mutable borrow

One option that we might explore in the future is passing the Ui as a parameter to the closure.

ui.with_id("lol", |ui2| {
  ui2.text("foo"); // ok, because we're borrowing ui2 and not ui
});

However, this most likely introduces a lot of variable shadowing in the user code, because the naming of the ui parameters would quickly get out of hand. So in practice the user code would probably look like this:

ui.with_id("lol", |ui| {
  ui.text("foo"); // ok, because we're borrowing the "inner ui", not the outer one
});

[aloucks]

Paired calls are much closer to the source API. I think it's better to try to mirror it as closely as possible instead of building a new and different API on top of it. Using the naming convention of the source API would also be beneficial as most examples and code are from the C++ ecosystem. (e.g. "Begin" vs "Window").

Also, have you considered dropping the im_str! macro? I think most of the heap allocations needed to convert &str to a null terminated string can be prevented by using stack allocated buffers and only falling back to heap allocated for larger strings. You can even macro all of this to use a thread_local heap allocated buffer per call site. I don't think there's going to be a significant performance loss for memcpying most of these small strings.

[yanchith]

Thank you for your work!

If we fully accept the fact that builders in this case are really just a way to handle default arguments, why not make ProgressBar an "optional argument struct" that just carries all the arguments that can be left to default values:

// Using custom arguments
ui.progress_bar(
    0.6,
    ProgressBar::default()
        .size([100.0, 12.0])
        .overlay_text(im_str!("Progress!")),
);

Readability-wise, builders seem simpler than this example. In it, there is a ui.progress_bar() and a ProgressBar - there are more items you need to "know about".

That leads me to my second point:

How about having the function in the Ui type, but as a function that doesn't need the reference:

FWIW, this approach has the added convenience/readability benefit of not having to import everything, but rather be able to access all UI elements from a single place. Of course, similar usage could achieved by not importing the names directly, but using imgui::ProgressBar::new(), but that is slightly longer and I am not sure many people prefer the style of module::{Type,function} to importing the names directly.