Fix and improve datamodel bindings

[espkk]

Rewrite datamodel binding so it works in both sides and variations of nested tables/arrays with rebind on rewrite

see #6

[espkk]

I found a couple of issues with this implementation, so it's not really ready for review. But I'm still interested in any thoughts on this

UPD: I think I've fixed all the issues I found. I'm not sure if it makes sense (and feasible) to add tests though

[LoneBoco]

@espkk Sorry it's taken so long. Been a very busy December for me. I've pushed some changes to fix some compilation errors, but you'll need to rebase this PR on main as it is blocked due to conflicts.

EDIT: Remember to pull latest on your fork so you get the updates I made.

[LoneBoco]

@espkk If you don't mind me attempting to force push into your own repo, I could try to do this rebase myself.

[espkk]

@espkk If you don't mind me attempting to force push into your own repo, I could try to do this rebase myself.

Sorry for long reply. Last time I was tinkering with it I spotted a big issue with data model lifetimes in lua, arising from the fact the lua GC can't track references inside userdata.
Have you faced anything similar? I'm wondering if it makes sense to change the way how data model is accessed/lifetime is managed/observed

More specifically, it probably only concerns events/datamodel events that access datamodel themselves. It leads to a circular dependency between datamodel->callback->datamodel invisible to GC. There are probably ways to avoid it, but it occurs naturally so I'm wondering if the design of datamodel holding actual references to callbacks is viable

[espkk]

I don't think this is an issue but note that both of these changes change complexity of a negative case from O(1) to O(N), where N is a string length

[LoneBoco]

Ah, good point, I forgot that it would have to calculate the length of the string.

[LoneBoco]

Sorry for long reply. Last time I was tinkering with it I spotted a big issue with data model lifetimes in lua, arising from the fact the lua GC can't track references inside userdata. Have you faced anything similar? I'm wondering if it makes sense to change the way how data model is accessed/lifetime is managed/observed

I don't think I've been using data models in as complex a way as you have. I've mainly used them to bind to internal structures, like an item list that I then construct a UI for with the data-for attribute. Everything I pass into Lua is either a raw pointer (since a std::unique_ptr holds the data for the lifetime of the program), or a std::shared_ptr.

More specifically, it probably only concerns events/datamodel events that access datamodel themselves. It leads to a circular dependency between datamodel->callback->datamodel invisible to GC. There are probably ways to avoid it, but it occurs naturally so I'm wondering if the design of datamodel holding actual references to callbacks is viable

And I haven't ever tried to hold a data model within another.

[espkk]

And I haven't ever tried to hold a data model within another.

It's not about holding data model within another, rather event handlers holding upvalues while being held by a uservalue themselves. I guess this is a generic issue with uservalues in lua, I'm just not sure how to work around it in case of rml: think of closing then reopening the interface. Unless you want the state to be never unloaded it needs to observe and destroy all the state associated with the rml document instance

[LoneBoco]

This is how I used it in one of my own projects:
https://github.com/LoneBoco/2drp/blob/1b4d99c8681d50631ebecc2236d0023b8308e2c7/src/client/ui/UIManager.cpp#L401

The classes are at the top. Here's ItemInstanceVariableDefinition:
https://github.com/LoneBoco/2drp/blob/1b4d99c8681d50631ebecc2236d0023b8308e2c7/src/client/ui/UIManager.cpp#L81

I create the item link with:

constructor.Bind("items", &game->Server->GetPlayer()->Account.Items);

And I use it like this:

<div id="container-items" data-model="items">
    <button type="button" data-for="item : items" data-attr-id="item.id" data-attr-name="item.name" data-attr-class="item.tags" onclick="Select(element)">
        <table>
            <tr>
                <td>
                    <div class="image-center"><img data-attr-src="item.image"/></div>
                    <span class="name">{{item.name}}</span>
                    <span class="count" data-if="item.type == 'stackable'"> x{{item.count}}</span>
                    <span class="variant" data-if="item.type == 'variant'"> *</span>
                </td>
            </tr>
        </table>
    </button>
</div>

When you talk about callbacks, are you talking about Event Callback Functions? I just don't quite understand what you are trying to do. It doesn't seem like RmlUi's data model is flexible enough to do certain things. Do you have a simple little code example of the scenario you are trying to describe?

[espkk]

@LoneBoco sorry for delay

I've added another fix for array insertions, and reverted complexity regression (if you don't mind)

This is how I used it in one of my own projects: https://github.com/LoneBoco/2drp/blob/1b4d99c8681d50631ebecc2236d0023b8308e2c7/src/client/ui/UIManager.cpp#L401

The classes are at the top. Here's ItemInstanceVariableDefinition: https://github.com/LoneBoco/2drp/blob/1b4d99c8681d50631ebecc2236d0023b8308e2c7/src/client/ui/UIManager.cpp#L81

Do you create data models only from C++. Perhaps I have a very different case - I only do basic initialization in C++, and then only interact with RmlUi from lua.

When you talk about callbacks, are you talking about Event Callback Functions? I just don't quite understand what you are trying to do. It doesn't seem like RmlUi's data model is flexible enough to do certain things. Do you have a simple little code example of the scenario you are trying to describe?

Yes. I actually didn't know about them before running into them in your code and I use it since then 😄

RmlSolLua/src/bind/Context.cpp

Lines 64 to 83 in ee9cff0

	if (value.get_type() == sol::type::function)
	{
	data->Constructor.BindEventCallback(
	skey,
	[skey, cb = sol::protected_function{value}, state = sol::state_view{table.lua_state()}](Rml::DataModelHandle, Rml::Event& event, const Rml::VariantList& varlist)
	{
	if (cb.valid())
	{
	std::vector<sol::object> args;
	for (const auto& variant : varlist)
	{
	args.push_back(makeObjectFromVariant(&variant, state));
	}
	auto pfr = cb(event, sol::as_args(args));
	if (!pfr.valid())
	ErrorHandler(cb.lua_state(), std::move(pfr));
	}
	}
	);
	}

The way how I got into the issue with GC/lifetime:

1. I create datamodel from lua and use hold it globally (in the isolated environment)
2. datamodel table has an event handler that is a closure with the datamodel used as an upvalue

so there's already a cycle
global → datamodel → handler → (upvalue) → datamodel
where datamodel is usertype

it could be something like

local datamodel = OpenDataModel("test", { counter = 1, event_test = function(event) datamodel.counter = datamodel.counter+1 })

and now

1. datamodel doesn't have observer interface (unlike document or other elements in RmlUi)
2. datamodel lifetime is not tied to document's

So without workarounds it can easily become either a leak or use-after-free after datamodel was destroyed. It also complicates hot-reload which I'm working on right now.
I asked about observability api but maybe there's a better way to handle that

[LoneBoco]

I've added another fix for array insertions, and reverted complexity regression (if you don't mind)
I don't mind.

Do you create data models only from C++. Perhaps I have a very different case - I only do basic initialization in C++, and then only interact with RmlUi from lua.
Yeah, I use the data models to expose dynamically changing elements from my engine. In this case, the list of items a player has.

You could always check and see how the official bindings deal with data models:
https://github.com/mikke89/RmlUi/blob/master/Source/Lua/LuaDataModel.cpp

From the comments:

// If you create all the Data Models from lua, you can store these LuaDataModel objects in a table,
// and call CloseLuaDataModel for each after Context released.
// We don't put it in __gc, becuase LuaDataModel can be free before DataModel if you are not careful.
// scalarDef will free by CloseLuaDataModel, but DataModel need it.

It looks like they expect you to manage the life cycle of the data model yourself and manually close it.

One thing they end up doing is storing a custom table inside Lua to keep track of the dependencies. If the data model doesn't have any observability, you could just observe the document (if you can, I haven't checked what can be observed or not).

The thing I'm not sure how to deal with is the closure holding onto a reference to the datamodel. You might be able to figure something out with std::shared_ptr and std::weak_ptr, but that will probably be a hard one.

[espkk]

You could always check and see how the official bindings deal with data models:

From what I understand this was an unfinished proof of concept, and the original author reimplemented it in a different way 😄
So it works but has too many limitations to be useful.

I thought about associating a data model with a specific document, but it loses some flexibility and adds potential overhead for reinit. If data model had observer interface, it probably could be done by just adding a thin non-owning wrapper with observer test.

Anyway, it's not really related to this PR... regardless of the solution, most likely it will be needed on the application side, rather than library

[LoneBoco]

These can be constexpr since std::string_view::starts_with is constexpr. Just a little micro-optimization. :)

[espkk]

It can, but it is only called in runtime context, so constexpr will be discarded in all cases
After all, it should be just just a couple instructions after inlining - size check + single character test
As a micro optimization we can do [[assume key.size() > 0]]; to potentially eliminate size check 🙂

[espkk]

Drafting this as I'm waiting for two changes in the upstream RmlUi to resolve lifetime issues and make datamodel truly dynamic (with lazy bindings)