Remove the lifetime from Ui

[sanbox-irl]

This is a very simple change which will lead to significant (possibly a massive amount, in fact) of breaking changes. I'll add to this PR some approaches around this to explain my reasoning.

What was changed in Ui

All this change does is convert Ui<'a> to Ui, a ZST with one zero sized field UnsafeCell<()>. We never use this unsafe cell, and it is optimized out (ie, size_of Ui is 0), but we need it to prevent the compiler to implement Sync for Ui (it does implement Send, but that won't matter for us, as seen further down).

Ui previously held three fields: a reference to the imgui-rs Context which wasn't used, a shared font holder reference, possibly, and, lastly, a vec for our scratch sheet for the im_str changes.

The imgui-rs Context reference was used in a few stack tokens which have since been rewritten to not require it, so this reference could be trivially removed.

The shared buffer was replaced with a toasty static mut, which we only allow &Ui to access. Since we make sure that Ui is not Sync, two threads cannot access this at the same time, and we can safely mutate through it.

The font handler has not been well handled -- right now, we simply assert that we CAN borrow the font before accessing the context, and if we can, we borrow it, and only now hand users the raw pointer wrapper to their font systems. I do not think that this was a well used feature, and I think that a combination of global variables can resurrect this usage. I would like to keep the Ui object as a ZST, mostly for simplicity, but this might force us to add some data to it.

So with all of those changes, Ui doesn't actually NEED any fields.

Understanding the lifetime of Ui

Ui was previously defined like Ui<'a>(&'a Context). This gave it the semantics of a &T, but since we never implemented Copy/Clone for Ui, it had semantics more similar to &mut T.

Most of the methods on Ui (and most user code which passes it around through routines), will take an &'ui Ui<'ui>. This is a strange lifetime and is slightly incorrect. In reality, its lifetime is &'a Ui<'b> where 'b: 'a, since any reference ('a) definition has a shorter lifetime than the lifetime of the thing it's pointing to (assuming Rustc is doing its job). However, in practice, this doesn't particularly matter, so flattening them to the same lifetime is fine. It would prevent returning data in certain methods correctly, but we never did that anyway. One place where that lifetime, however, would get problematic would be the introduction of mutability. This PR fundamentally is paving the way for a larger PR that I am working towards to add mutability (and its various constructs) into imgui-rs.

Okay, but wait, didn't we need that lifetime?

Yes, and also no. We get all the effect of the lifetime by...never giving out Ui. Now, frame (I also added new_frame since frame was just a needlessly strange name) returns an &mut Ui. There is, as a result, only this single lifetime that we need to worry about.

Since Ui is a ZST, it can just be made out of the void. We need the lifetime though, so we store it as a field on Context, so we're locking context for the duration of Ui's existence.

This just works. I suspect most users won't notice this lifetime change directly, but will notice it in their parameters, which will need to have the lifetime removed.

This does have on impact -- because we lose the ability to drop Ui in user land code, render has been moved from Ui to Context. Via NLL, this prevents Ui from being used after .render (since .render takes an &mut self). However, it does mean additionally that we run an increased risk of, in certain cases, forgetting to end the frame, trigger some dear imgui asserts. I've bandaged around this by conditionally running end_frame in the Context's drop code, but in certain cases (ie, if you just forget to call .render in your main loop), we'll still end up getting an ImGui drop.

[thomcc]

I'm extremely positive about removing <'ui> but much less convinced about making Ui into a ZST by replacing its fields with globals.

[thomcc]

Almost certainly no point in both static mut and UnsafeCell here.

Also, I don't think your arguments around simplicity really make sense that much here. I think the important part of the Ui<'ui> to remove was just the lifetime. This being a ZST doesn't really seem to help anything, and it does make adding any additional state Ui much harder, it has to be global variables.

[thomcc]

Oh, also, "it does implement Send" is this true? If it is this code is unsound. I though UnsafeCell was !Send by default too, though.

[thomcc]

Hm, seems so. I think this isn't sound because the existence of &Ui on a thread isn't strong enough to prove that there's been sufficient synchronization of the writes behind BUFFER.

[sanbox-irl]

There's really no reason it has to be a ZST. I do think the compiler can do some fun magic to erase the reference but that's mostly me having fun.

How do you think the current code breaks its guarantees? Since I'm not trying to correctly synchronize, just prevent, multithreading, I thought this would be sufficient

[dbr]

Hmm I'm not aware of the backstory behind Ui's lifetime or the benefits of it's removal, but moving towards ui: &mut Ui sounds like a good plan

Via NLL, this prevents Ui from being used after .render (since .render takes an &mut self). However, it does mean additionally that we run an increased risk of, in certain cases, forgetting to end the frame, trigger some dear imgui asserts

If I understand right, This seems a bit of a step backward, as ignoring the underlying implementation, the old structure of:

let context = imgui::Context::new();
let ui = context.new_frame();
ui.button("...");
let draw_data = ui.render();

made a lot of sense to me - it makes it clear the draw data was tied to the stuff I did with ui - and once Ui::render(self, ... is called I couldn't reasonably expect something like this would work:

let context = imgui::Context::new();
let ui = context.new_frame();
ui.button("...");
let draw_data = ui.render();

// Nonsense:
ui.button("oh no wait please add this button too");
let data = ui.render(); // Wont compile, Ui was moved (.. I think?)

If I understand correctly, the above now looks like:

let context = imgui::Context::new();
let ui = context.new_frame();
ui.button("...");
let draw_data = context.render();

// The above sort-of implies I can still use `ui`
ui.button("!");
let draw_data = context.render(); // Get data but with an additional button, or.. something

[sanbox-irl]

nice, good question at @dbr. I was worried about that too, BUT since new_frame takes &mut self in the context...

let context = imgui::Context::new();
let ui = context.new_frame();
ui.button("...");
let draw_data = context.render();

ui.button("!");

this will not compile. Instead we get an error on context.render informing us that we still have an &mut Context borrow because of the &mut Ui we still have active.

If we start a new frame after context.render (and thus refresh our borrow of &mut Ui to have a new lifetime), then not only is that code LEGAL, but it's also actually completely correct. That's just doing two frames of imgui manually.

The lifetime of the data in render actually was the Context the whole time -- it returned an &'ui DrawData, relating it to the Context originally. That was important then just like it is now, since you (in this new system) cannot do this either:

let context = imgui::Context::new();
let ui = context.new_frame();
ui.button("...");
let draw_data = context.render();

let ui = context.new_frame(); // error here -- we still have a borrow of `draw_data` that's used later, so we can't take `&mut Context`
my_renderer.render(draw_data);
ui.button("!");

So in total, we do not lose our lifetime backing of Ui. I agree that that is extremely important