FyroxEngine / Fyrox

3D and 2D game engine written in Rust
https://fyrox.rs
MIT License
7.48k stars 339 forks source link

Message bubble redirection experiment. #633

Closed b-guild closed 2 months ago

b-guild commented 2 months ago

This is an experiment in making it possible for a message handler to change where the message bubbling will proceed. This allows for bubbling to go to places other than a node's parents, and makes it possible for a popup to have an "owner" which will receive messages that bubble up from the popup, eliminating the need for previewing messages, without breaking the way any currently existing widgets work.

This should mean that we could add many more context menus without any concern over adding more widgets to the preview message list. For example, we might give every TextBox a context menu for copy, cut, paste, and it would not require every TextBox to receive preview messages.

I also made it so that widgets only search for context menus when the right-click message is not handled. This makes it possible for the Inspector to do its own right-click handling and set the target for its context menu and then open the context menu manually, thus avoiding the need for the awkward hit_test that it used to do in order to figure out the target of the menu later.

b-guild commented 2 months ago

This is quite controversial change. The more I think of it, the more potential issues I see.

I agree that it is experimental and potentially controversial, but it should be a feature that only affects widgets that choose to use it. I am curious to hear more about the issues you see.

This significantly convolutes message routing logic. Breaking the guarantees that the message will always bubble up through the real chain of parent widgets.

It does break that guarantee, but a popup from a widget is very much akin to a child of that widget. Due to technical reasons the popup cannot actually be a child of the widget, but the widget still wants to handle messages for the popup as if the popup were a child. It seems that to allow messages to bubble up to the owner is a clean resolution to getting messages where they need to be handled.

In case of context menus there could be any number of owners, and I think this forwarding won't help either (unless you will change the owner on the fly to the widget with the context menu when the menu's popup is opening).

That was the idea that I had in mind. A single popup could have many owners, but only one owner at a time. Each time a popup opens, it opens for some particular owner due to some particular mouse click on some particular widget. It seems reasonable that as part of the process of opening the context menu, the owner could be set as appropriate.

If the only thing that bothers you is the hit_test stuff, then the real reason why it is done like so is my laziness. I was lazy to pass the context menu in property editor context and manually assign it for all of the property editors.

Yet that is unnecessary. If the Inspector widget handles the MouseDown message, then it can store the destination of that click and use it to look up which editor the user was clicking on. There is no need to set the context menu onto every editor.

But I realize now as I write this that doing it this way could create an issue in that having the Inspector handle the right-click means that the normal context menu opening code will not run, so if any widgets inside an editor were to have their own context menus, those would not open, and only the Inspector context menu would open instead.

I feel like it would be better if each widget that needs a context menu would manually open its own context menu and set the MouseDown message to handled so that parents do not try to use it. Waiting until after the bubbling to determine which context menu to open seems contrary to the spirit of the message system.

Previewing messages is fast, since in typical conditions (30-50k active widgets) the only few of them are actually previewing messages (usually it measured in tens of widgets with the given amount of active widgets).

If we had more context menus and every widget with a context menu needs to preview messages, then it could be that many widgets will be previewing messages. Imagine if every TextBox has a context menu, and there could be dozens of text boxes on the screen at once. We could also have context menus for other widgets as well, maybe even make it so that having context menus is normal for almost all widgets. By letting popups redirect their messages to their owners, we can give context menus to as many widgets as we please without concern for the number of widgets that are previewing messages.

mrDIMAS commented 2 months ago

So, I re-read the code once again and thought about this forwarding approach and it seems to be ok. But I still find this manual menu opening quite hacky. As you already spotted, this will interfere with any custom context menu on the widgets inside the inspector. So I think I can accept message forwarding alone, but menu handling should be fixed. It think it could be easily fixed by wrapping each property editor into a dummy widget, that will have menu specified. This way any custom menu on a property editor will work fine, and if there's none - the dummy widget's menu will be used instead (since the UI searches up on tree for potential menus to open). By this approach you don't need to specify the menu on per-editor basis, the dummy widget will do the job and it will be easy to distinguish from which widget this menu was invoked.