[rmkit][ui] Widget ownership model

[mrichards42]

What's the "correct" way to keep references to widgets? It looks like Scene ends up taking ownership when you call add, which makes sense, since you need the widgets to live at least as long as the scene. Assuming a single main scene which lasts the entire lifetime of the program, it seems like it doesn't matter all that much, so storing raw pointers is perfectly reasonable, e.g.

class App {
public:
  ui::Text * textWidget;
  App() {
    auto scene = ui::make_scene();
    textWidget = new ui::Text(...); // nobody owns this yet
    scene->add(myText);             // now scene owns this
    ui::MainLoop::set_scene(scene); // scene owned by MainLoop, so textWidget won't be destroyed
  }
};

I'm getting a little tripped up with widgets that use overlays, like Dialog. I think Dialog and Scene have a circular reference? I added some logging and never saw a Dialog get destroyed, unless I deleted it myself, which led to unpredictable behavior (since Scene still thought it owned the dialog).

// Like other widgets, this starts out unowned
auto dlg = new Dialog(...);

// Showing a dialog does roughly this:
dlg->scene = ui::make_scene(); // scene owned by dlg
dlg->scene->add(dlg);          // dlg owned by scene
ui::MainLoop::show_overlay(dlg->scene); // scene owned by MainLoop

[raisjn]

the intent is that Scene takes ownership by using shared_ptr when a raw pointer is passed in, but Scene can also take a shared_ptr<Widget> to add(). the current ownership model is not well thought out and i'm not sure what should happen with Dialog+Scene, would appreciate advice.

is this because you are constructing / deleting new dialogs on a per game basis? i think the original intent with dialog was that an app would construct all the dialogs it needs at initialization and store references to them. (so they never get destroyed)

EDIT: perhaps you could say that since a dialog is owned by the app and the app usually owns scenes, the Dialog is a sort of custom scene?

[mrichards42]

is this because you are constructing / deleting new dialogs on a per game basis? i think the original intent with dialog was that an app would construct all the dialogs it needs at initialization and store references to them. (so they never get destroyed)

I can probably get away with creating a single copy of each dialog I need and updating them when the game changes. It just occurred to me that a dialog-heavy app might need to delete dialogs at some point, and I didn't see a way for that to happen, but I don't think that's a big problem for my needs, at least not yet.

the intent is that Scene takes ownership by using shared_ptr when a raw pointer is passed in, but Scene can also take a shared_ptr to add(). the current ownership model is not well thought out and i'm not sure what should happen with Dialog+Scene, would appreciate advice.

(Full disclosure, I'm fairly new the nuances of using smart pointers, most of the C++ code I've written was pre-11, and I didn't make much use of auto_ptr or boost). I like the idea that a Scene and all its Widgets could be disposable if you don't need them. And if you do need to keep them around, you can use shared_ptr to keep the references alive.

I think that's mostly possible today actually -- if you need a throwaway Scene, you can create it on the heap, and MainLoop will take ownership, then delete it when you call set_scene with something else.

I think you could get the same behavior with Dialog if it kept a weak_ptr instead of a shared_ptr to InnerScene, to break the Scene -> Dialog -> Scene loop. The interface as it stands would be a little rough, since you'd have to keep calling scene.lock() to do anything with it, but maybe the dialog functions could have a regular Scene as the first argument, instead of using this->scene?

[raisjn]

I think you could get the same behavior with Dialog if it kept a weak_ptr instead of a shared_ptr to InnerScene, to break the Scene -> Dialog -> Scene loop. The interface as it stands would be a little rough, since you'd have to keep calling scene.lock() to do anything with it, but maybe the dialog functions could have a regular Scene as the first argument, instead of using this->scene?

1. can we manually break the loop by deleting the scene out of the dialog when its no longer used? but i guess the whole point is to do this automatically with shared_ptrs when we realize the dialog falls out of scope.

2. i don't see how to use weak_ptr, here - if we make dialog.scene a weak_ptr, would it get collected almost immediately (or who owns the scene?). i guess we can add a new add() function which tells the Scene to not construct a shared_ptr (so dialog would be a weak reference inside the Scene), but then scene would hold two sets of references: a set of weak and a set of shared ptrs (i think). is there another way?

[mrichards42]

i don't see how to use weak_ptr, here - if we make dialog.scene a weak_ptr, would it get collected almost immediately (or who owns the scene?). i guess we can add a new add() function which tells the Scene to not construct a shared_ptr (so dialog would be a weak reference inside the Scene), but then scene would hold two sets of references: a set of weak and a set of shared ptrs (i think). is there another way?

No, you're totally right that weak_ptr dialog->scene won't do it. I'm not even sure a weak scene->dialog would work, since then Dialog doesn't get collected when Scene is destroyed. I wonder if it's actually the Scene we care about -- so if you want to keep the "dialog" alive, you retain your own shared_pointer to the scene? Then there are no cycles to worry about.

class DialogBackground : ui::Widget() {
  void render() {
    fb->draw_rect(x, y, w, h, WHITE, true);
    fb->draw_rect(x, y, w, h, BLACK, false);
  }
};

class Dialog : InnerScene {
  public:
  DialogBackground * background;
  DialogScene(x, y, w, h) : InnerScene() {
    background = new DialogBackground(x, y, w, h);
    add(background);
  }

  void build_dialog() {
    add(new Text(0, 0, background->w, background->h, "Hello world");
  }

  void show() {
    if (!built) {
      build = true;
      build_dialog();
    }
    // MainLoop takes ownership
    ui::MainLoop::show_overlay(this);
  }
};

A quick sketch of the interface I'm thinking of, just to wrap my own head around it:

// Destroy this dialog (scene) as soon as it's hidden
(new Dialog())->show();

// Keep the dialog (scene) around since I'm keeping track of it
this->dlg = make_shared<Dialog>()
this->dlg->show();

^^ And that could all be wrapped up in a shared_ptr typedef like Scene.

It looks like both Qt and wx have you allocate modal dialogs on the stack, and run a separate event loop before returning, e.g.

if (wxMessageBox("Hello world") == wxOK) // new event loop runs until the dialog is closed
  do_something();

^^ that would be pretty slick, but might be too complicated.

[raisjn]

dialog as innerscene makes sense to me. the separate event loop is interesting but i don't want to implement that just yet

[raisjn]

i spent some time last night looking at the hamburger menu and at other dialogs to get an idea of what's going on. to summarize (for my own benefit):

• dialog creates a scene and then populates it
• dialog holds onto scene
• when dialog is shown, it tells the main view to display the scene
• when dialog is destroyed, it still exists in scene and it still has reference to scene, so nothing gets collected

---

one way this could be fixed is: the dialog always creates a new scene when it wants to show itself (instead of holding onto one), but not sure if that's good.

another way to solve this would be to make the dialog have a weak reference to the scene by using a handle to the scene in the dialog. for example,

def build_dialog():
  scene := ui::get_scene(self.scene_token)
  ...

~Dialog():
  ui::release_scene(self.scene_token)

the last way, described above, is to have dialog be a scene (which is also fine with me)

[mrichards42]

Since I opened this originally I've found myself gravitating toward a pattern for scenes in general, both scenes that are used as overlays and those that aren't:

• There's a "container" class that doesn't derive from anything
• That class has a Scene
• The Scene owns any widgets created for the container class, but the container can store raw pointers if it needs to

e.g.

class MySceneContainer {
public:
    ui::Scene scene;
    ui::Text * label;
    ui::Button * ok_btn;

    MySceneContainer(...)
    {
        scene = ui::make_scene();
        scene->add(label = new ui::Text(...));
        scene->add(ok_btn = new ui::Button(...));
    }

    void show()
    {
        ui::MainLoop::set_scene(scene);    // if it's a main scene
        ui::MainLoop::show_overlay(scene); // if it's an overlay
    }
};

And then for "main" scenes, the App class stores a unique_ptr to that container class. For temporary dialogs, right now I have those owned by the container (also as a unique_ptr), and then when the dialog is hidden, I'm clearing out both the container's pointer and the MainLoop's overlay so the dialog gets destroyed (like this).

Specifically, the places I'm using this pattern now:

• ChooserScene -- one of the main scenes
• GameScene -- the other main scene
• GameMenu -- an overlay

[raisjn]

nice! is there anything rmkit can do to make the pattern easier / used in more cases?

i think there could be ui::MainLoop::release(scene) (which does the simple check you have linked), but wonder if there is more

[mrichards42]

👍 yeah, MainLoop::release sounds like it would help. This SceneContainer or whatever could probably be a base class (with things like show(), is_shown(), hide(), etc.), but it's only a few methods, so 🤷