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

Reduce global state #489

Closed
Thalhammer opened this issue Jul 10, 2023 · 4 comments
Closed

Reduce global state #489

Thalhammer opened this issue Jul 10, 2023 · 4 comments
Labels
discussion Meta talk and feedback enhancement New feature or request

Comments

@Thalhammer
Copy link
Contributor

RmlUI should not depend on global state as heavily as it currently does.

I don't see a reason why

  • SystemInterface
  • RenderInterface
  • FileInterface
  • FontEngineInterface
  • The Plugin registry

need to be global state. RenderInterface used to be per context and was moved to be global only with (as far as I can tell) no prior warning or hint about it to users, which caught them (or at least me) totally off guard.

The commit 0bbd019 that does so outlines the reasoning:

  • The feature added considerable complexity, since several types of render state needed to store their state per render interface.
    This wouldn't be an issue if there would not be so much global state. Instead of removing the ability to use different interfaces, move the stuff that depends on the render interface into the context (or the render interface).

  • This complexity would have become more pronounced with upcoming changes for advanced render effects.
    Not if it is contained in the scope it belongs to.

  • It was easy to make bugs where render resources (e.g. textures) would be freed in the wrong render interface.
    Not if the render resources are stored where they belong (in the context that created them) instead of globally. How are you supposed to free a wrong resource if you don't even see it.

  • It was difficult to test for the presence of the above bugs.
    Move the global stuff into the context and the above bugs are either gone (because you only see "your" data) or cause obvious compile errors.

  • No tests existed already, it is very likely that bugs were already present.
    I have used multiple render interfaces for close to 3 months productive now and haven't hit a single one. So they either hide very well or there are no significant ones. But changing everything global wont make those bugs disappear, it just hides them better and makes them even harder to detect/debug.

I propose an alternative solution:
Remove all (most) of the global state and integrate it into the context and existing interfaces.
This gets rid of all the complexity mentioned above and makes RmlUI way more flexible with respect to using it in multiple windows/contexts from the same app (#407, #477, #480, #481, #307 (comment)).

I'd be happy to help this conversion, as there is nothing else quite like RmlUI but I feel like instead of making it easier and more versatile to use, RmlUI goes the exact opposite way.

@mikke89
Copy link
Owner

mikke89 commented Jul 10, 2023

First of all, I am planning to reintroduce the per-context render interface with the upcoming effects branch. More of the render state has been collected into the context here, and perhaps we can make a better solution than previously. Users should expect several breaking changes as we are moving towards RmlUi 6.0, I'd recommend to stick with the releases for users that want stability.

Then, overall I agree with you, it would be good code hygiene to tie the interfaces to specific resources, the context in particular. What you are asking for is a lot of work though, and I need to make priorities. The previous solution was pretty much a glorified global, which is why I decided to remove it at the time.

If you are willing to help out, I'd be happy to see an effort into making this conversion. I will seriously consider such a change. Generally avoiding any breaking changes would be a priority.

@Thalhammer
Copy link
Contributor Author

Generally avoiding any breaking changes would be a priority.

Not sure if its doable without breaking changes (even if the interface doesn't change, the behaviour in some edge cases, will probably change), but there already seem to be breaking changes (the render interface was expanded) so why not do it right while we are at it.

What you are asking for is a lot of work though, and I need to make priorities.

I know and I fully understand this. Its kinda impressive how much time you manage to spend on this project as is, but I'd be happy to help if there is interest in it.

@mikke89
Copy link
Owner

mikke89 commented Jul 11, 2023

Some breaking changes are acceptable, but try to keep it low when possible. Most of the new render interface features will be opt-in, although there will be a few breaking changes there.

Thanks for the kind words, and I'm certainly interested and looking forward to seeing how this turns out.

@mikke89
Copy link
Owner

mikke89 commented Aug 26, 2024

Cleaning up some old issues. For the RmlUi 6.0 release, the render interface is again tied to the context, which partly addresses the concern here. Since there hasn't been much activity on this issue I am closing this one, any further proposals for reducing the global state should be discussed in new issues or PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Meta talk and feedback enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants