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

Support explicit DataTypeRegister setup for DataModel. #409

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

eugeneko
Copy link
Contributor

@eugeneko eugeneko commented Feb 2, 2023

Per my comments in Gitter.
This ability to manage lifetime of the registry solves the crash that happens on DLL unload when this DLL creates RmlUI models.

Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First I want to say, in general I'd be worried about using the data model functionality across DLL boundaries. It depends on uniquely generated type ids, which might conflict across boundaries.

Other potential approaches could be:

  • Construct separate contexts for each DLL.
  • Make a Context::ResetDataModels function (which also clears the type registry).
  • With your PR, one can be careful about using one type registry per DLL. I guess perhaps this is your intention? If so, should probably document this somewhere. I guess this would also take care of my initial worry above, although at the expense of needing to be careful when constructing new data models.

With that said, this proposal is not very invasive, so I would be okay with adding it - please see the suggested changes.

Include/RmlUi/Core/Context.h Show resolved Hide resolved
Source/Core/Context.cpp Outdated Show resolved Hide resolved
Source/Core/DataModel.h Show resolved Hide resolved
Source/Core/DataModel.h Outdated Show resolved Hide resolved
@eugeneko
Copy link
Contributor Author

eugeneko commented Feb 3, 2023

First I want to say, in general I'd be worried about using the data model functionality across DLL boundaries. It depends on uniquely generated type ids, which might conflict across boundaries.

With your PR, one can be careful about using one type registry per DLL. I guess perhaps this is your intention? If so, should probably document this somewhere. I guess this would also take care of my initial worry above, although at the expense of needing to be careful when constructing new data models.

Yep, my intention is to allow users to work within DLL boundaries instead of automatic and invasive magic in the Context.

I am not sure about having multiple contexts, because we may have multiple DLLs with multiple isolated data models, that all render to the same window as independent documents. Multiple contexts will probably cause more separation than I want. If documents live in separate contexts, I cannot really have them working together with correct focus transitions and stuff, right?

About your comments, I will revisit this PR at some point.

@eugeneko
Copy link
Contributor Author

eugeneko commented Feb 3, 2023

About your concerns regarding DLL boundaries, I like the approach of EnTT.
They have their fair share of template IDs, but they provided alternative API that is module-agnostic and therefore it works reliably regardless of the boundaries. EnTT allowed users to provide type IDs manually by tagging the types somehow.
They may have changed something since I last used it across the boundaries.

It's not crucial for me to have now, but it is a nice thing to have boundary-agnostic API.

@mikke89
Copy link
Owner

mikke89 commented Feb 3, 2023

Yeah, multiple contexts might be complicated if you need things to interact, or documents to overlap in arbitrary order. I think your approach makes sense so I'm fine with this direction. As for boundary-agnostic API, that sounds good too, although it's not a priority for me personally.

I fixed some build warnings in the master branch, so you might want to rebase this when you make your changes.

@eugeneko
Copy link
Contributor Author

@mikke89 Should be good to go now

@mikke89 mikke89 merged commit c50911b into mikke89:master Mar 23, 2023
@mikke89
Copy link
Owner

mikke89 commented Mar 23, 2023

Looks good, thanks for finishing up this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants