Multiple WPFControls causes stack overflow with KeyUp/DownEvent Raise event

[lazerW0lf]

Problem: I am using version 4.2.2 and am instantiating multiple GLWpfControls (my application can have multiple tabs with an OGL-based scene) and am encountering a stack overflow upon launching the other GLWpf controller. I've found the root cause of this to be related to the OnKeyUp/OnKeyDown functions that ultimately call RaiseEvent.

Some possible solutions: I've worked around this in my local repo by checking if the event is handled or not. In my KeyUp/Down callbacks I've set the event's Handled property to true and I made the following edit:

In GLWpfControl.cs:

        internal void OnKeyDown(object sender, KeyEventArgs e)
        {
            if (e.OriginalSource != this && !e.Handled)
            {
                KeyEventArgs args = new KeyEventArgs(e.KeyboardDevice, e.InputSource, e.Timestamp, e.Key);
                args.RoutedEvent = Keyboard.KeyDownEvent;
                // \todo I am commenting out because I get an overflow here...
                RaiseEvent(args);
            }
        }

Note that I added the && !e.Handled and am flagging the event as Handled=true in my own key callbacks.

My working theory is that two GLWpfControllers get into an infinite loop by catching/raising the event until the stack overflows.

At the very least, could we get some kind of user-level setting to enable/disable the event handling to avoid this. Or even a setting in Start()'s to protect from this block:

EventManager.RegisterClassHandler(typeof(Control), Keyboard.KeyDownEvent, new KeyEventHandler(OnKeyDown), true);
EventManager.RegisterClassHandler(typeof(Control), Keyboard.KeyUpEvent, new KeyEventHandler(OnKeyUp), true);

Something like

            if ( _settings.RegisterKeyHandlers )
            {
                EventManager.RegisterClassHandler(typeof(Control), Keyboard.KeyDownEvent, new KeyEventHandler(OnKeyDown), true);
                EventManager.RegisterClassHandler(typeof(Control), Keyboard.KeyUpEvent, new KeyEventHandler(OnKeyUp), true);
            }

[xhuan8]

@lazerW0lf I have the same problem, any workaround?

[lazerW0lf]

Yeah,

Build the project from source and then you have a few options. I think I outlined a few of them in my issue. Let me know if any of those work for you.

I can get you more details tomorrow if you are still stuck.

Thanks, Jonathan

On Thu, Jul 14, 2022 at 11:45 PM xhuan8 @.***> wrote:

@lazerW0lf https://github.com/lazerW0lf I have the same problem, any workaround?

— Reply to this email directly, view it on GitHub https://github.com/opentk/GLWpfControl/issues/82#issuecomment-1185230008, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZ6I5XDIQKBFLTWKXUUOMADVUECHFANCNFSM522QUHGQ . You are receiving this because you were mentioned.Message ID: @.***>

[xhuan8]

thanks, the problem is clear enough. the control should provide some method to unregister event handles

in my case, I only need one instance, but call Start() multiple times on window initialization I'll try keep the instance of control and avoid multiple Start() calls

[NogginBops]

This happens because the way we register currently to the KeyDown and KeyUp events. What we are doing currently is not the "correct" way, it's more of a workaround which seems to break badly in this example.

I think this issue is that the control is not considered "Focusable" and should therefore not be able to get keyboard events. I'm currently experimenting with a proper fix to this but I've not found a full solution as of yet.

[MingGuys]

I have same problem.If two windows contains GLWpfControl.Any key press will cause stackoverflow exception.for a single window ,It's Ok,but reopen this window .press any key,Then stackoverflow exception.

[MingGuys]

This happens because the way we register currently to the KeyDown and KeyUp events. What we are doing currently is not the "correct" way, it's more of a workaround which seems to break badly in this example.

I think this issue is that the control is not considered "Focusable" and should therefore not be able to get keyboard events. I'm currently experimenting with a proper fix to this but I've not found a full solution as of yet.

We handle keyboard and mouse event in Render methods.What about don't handle event by control itself?

[NogginBops]

I have not looked at this in too great of detail, but I believe #90 should at least create a workaround for this issue.

[MingGuys]

I have not looked at this in too great of detail, but I believe #90 should at least create a workaround for this issue.

Thanks .I gonna check

[nrankin18]

I also have this issue in version 4.2.3. Setting RegisterToEventsDirectly and CanInvokeOnHandledEvents to false on my controls seem to fix it though.

[MikiraSora]

I also have this issue in version 4.2.3.

[Xerxes004]

I have this issue as well. Setting RegisterToEventsDirectly and CanInvokeOnHandledEvents both to false also worked for me as @nrankin18 mentioned above.

[BaronWilliams]

Firstly, thanks for creating this control. It's very useful.

There are 2 ways to fix this issue.

THE FIX: OPTION A

The simplest fix is to set RegisterToEventsDirectly to false as suggested earlier. Setting RegisterToEventsDirectly to false simply causes the EventManager.RegisterClassHandler code to never be called. The EventManager.RegisterClassHandler code and it's associated event handlers have bugs in them, which I outline below in OPTION B.

With RegisterToEventsDirectly set to false, you can set Focusable="True" for the control if you need to get key events. This has worked for me without any problems. However, if you need your control to get key events when it doesn't have focus, then this solution is not viable.

THE ISSUES IN THE CURRENT CODE

There are 3 problems related to the event system that need to be addressed:

• EventManager.RegisterClassHandler is called for each instance. This is incorrect. It should only be called once per class type, because the registration is permanent, and doesn't go away when the class is destroyed. This should preferably be in a static constructor for the class. That is the correct usage. But in the code it's called in an instance member. Called in an instance member each time, would cause it to get triggered 100 times for a control if that control's window was closed and reopened 100 times. Which is not the behaviour we want.
• EventManager.RegisterClassHandler parameter Delegate handler is an instance member. It should be a static member because it registers the delegate for the class type permanently, not per instance of a class. Putting an instance member as the delegate is incorrect usage.
• Inside the event handlers for OnKeyDown and OnKeyUp, the code checks if "e.OriginalSource != this" before recycling the event. This is not correct, and causes the stack overflow when more than 2 controls are used, or if the control is closed and reopened in a new window. Because the code registered handlers permanently, it was possible for an unloaded control to send the event, and the unloaded control's "this" would not be the same as the current control's "this" so the check would fail and the loaded control would recycle the event to the unloaded control, and the unloaded control would recycle the event to the loaded control, and on and on, until you get a stack overflow.

THE FIX: OPTION B

If for some reason you want to use the existing event system, or you don't want your control to be focusable, but still want key events, then the current event system's flaws need to be fixed. I have a fix for it here.

EventManager.RegisterClassHandler should be used in a static constructor normally, but for this code it's not and so it should be as follows (note that areEventsRegistered is a static bool added to the class and set to false initially) if used inside an instance member such as Start:

            if (!areEventsRegistered )
            {
                areEventsRegistered = true;
                EventManager.RegisterClassHandler(typeof(Control), Keyboard.KeyDownEvent,   new KeyEventHandler(OnKeyDown), true );
                EventManager.RegisterClassHandler(typeof(Control), Keyboard.KeyUpEvent,     new KeyEventHandler(OnKeyUp),   true );
            }

The check for areEventsRegistered prevents more than 1 call to EventManager.RegisterClassHandler from occurring. Remember that EventManager.RegisterClassHandler creates a permanent event handler that lives for the entire lifetime of the application, not the class that called it.

The other issue, the actual cause of the stack overflow, was how the code decided if the event should be recycled or not. The check it used would only work for 1 instance of the control per application lifetime.

This is how I changed the OnKeyDown and OnKeyUp code to fix the stack overflow error (note that the handler is static, as it should be):

        internal static void OnKeyDown(object sender, KeyEventArgs e)
        {
            foreach (var loadedGLWpfControl in loadedGLWpfControls)
            {
                if
                (
                    (e.OriginalSource.GetType() != loadedGLWpfControl.GetType()) &&
                    (loadedGLWpfControl.IsFocused || loadedGLWpfControl.EnableGlobalEvents)
                )
                {
                    KeyEventArgs args = new KeyEventArgs(e.KeyboardDevice, e.InputSource, e.Timestamp, e.Key);
                    args.RoutedEvent = Keyboard.KeyDownEvent;
                    loadedGLWpfControl.RaiseEvent(args);
                }
            }
        }

Note the loadedGLWpfControl.EnableGlobalEvents check. This is there so that if you need your control to get the key event even when it doesn't have focus, then you set this to true.

If loadedGLWpfControl.EnableGlobalEvents is set to false, then your control only gets the event if it has focus using the loadedGLWpfControl.IsFocused check (for this to work your control must have Focusable="True").

So there are now 3 additional variables added to the class. I also removed CanInvokeOnHandledEvents and RegisterToEventsDirectly from my copy of the code since I don't need them.

        private static List<GLWpfControlM> loadedGLWpfControls = new List<GLWpfControlM> ();
        private static bool areEventsRegistered = false;
        /// <summary>
        /// Set to true to allow OnKeyDown and OnKeyUp to receive global events.
        /// <para/>
        /// 
        /// Set to false to allow OnKeyDown and OnKeyUp to receive events only
        /// when the control <see  cref="UIElement.IsFocused"/> is set the true.
        /// </summary>
        public bool EnableGlobalEvents { get; set; } = true;

The static loadedGLWpfControls member is maintained by adding code to the existing Loaded and Unloaded handlers in the control as follows:

...
            Loaded += (a, b) => {
                loadedGLWpfControls.Add(this);
                InvalidateVisual();
            };
...

        private void OnUnloaded()
        {
            loadedGLWpfControls.Remove(this);
            _renderer?.SetSize(0,0, 1, 1, Format.X8R8G8B8);
        }

With these changes, the stack overflow bug is fixed and with EnableGlobalEvents set to false you have the option of your control receiving key events only when it's in focus, or with EnableGlobalEvents set to true your control always receives key events even when not in focus, which can be useful in some cases.

[NogginBops]

With #86 we no longer call EventManager.RegisterClassHandler so this should not be an issue after that is merged.

[toolateralus]

I still have this issue. I was on version 4.8.1 that i acquired from nuget. I just used the workaround RegisterToEventsDirectly = false;

note: I updated to the package and its fixed. oops