[UI] Button click events are raised inside of Draw()

[Eideren]

Release Type: Github

Version: Latest as of writing

Platform(s): Any

Describe the bug
Right now UIRenderFeature.UpdateToucheEvents which calls ButtonBase.Click() runs within UIRenderFeature.Draw().
This is problematic as some parts of the architecture expects game-state mutation to happen within Update while Draw prepare this new state to be drawn, sometimes concurrently.
For example, changing SceneRendererCollection.Children from within this scope will throw as this list is processed higher up the stack inside of a foreach.
Buttons are often used as a way to switch between game states; loading in a new scene, changing graphics settings, etc. it is one of the area where those issues would be more prevalent, so I think it warrants delaying or moving it into update to avoid those issues.

We have had discussions regarding revamping some of the UI, solving this could be part of that effort, pinging @MechWarrior99 just in case

To Reproduce

MyButton.Click += (sender, args) => ((SceneRendererCollection)game.SceneSystem.GraphicsCompositor.Game).Children.Add(new ClearRenderer());

Log and callstacks

System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.List`1.Enumerator.MoveNext()
   at Stride.Rendering.Compositing.SceneRendererCollection.DrawCore(RenderContext context, RenderDrawContext drawContext) in I:\stride\sources\engine\Stride.Rendering\Rendering\Compositing\SceneRendererCollection.cs:line 28
   at Stride.Rendering.Compositing.SceneRendererBase.Draw(RenderDrawContext context) in I:\stride\sources\engine\Stride.Rendering\Rendering\Compositing\SceneRendererBase.cs:line 39
   at Stride.Rendering.Compositing.GraphicsCompositor.DrawCore(RenderDrawContext context) in I:\stride\sources\engine\Stride.Engine\Rendering\Compositing\GraphicsCompositor.cs:line 232
   at Stride.Rendering.RendererBase.Draw(RenderDrawContext context) in I:\stride\sources\engine\Stride.Rendering\Rendering\RendererBase.cs:line 51
   at Stride.Engine.SceneSystem.Draw(GameTime gameTime) in I:\stride\sources\engine\Stride.Engine\Engine\SceneSystem.cs:line 234
   at Stride.Games.GameSystemCollection.Draw(GameTime gameTime)
   at Stride.Games.GameBase.Draw(GameTime gameTime)
   at Stride.Games.GameBase.RawTick(TimeSpan elapsedTimePerUpdate, Int32 updateCount, Single drawInterpolationFactor, Boolean drawFrame)
   at Stride.Games.GameBase.RawTickProducer()
   at Stride.Games.GameBase.Tick()
   at Stride.Games.GamePlatform.Tick()
   at Stride.Games.GamePlatform.OnRunCallback()
   at Stride.Games.GameWindowWinforms.<>c__DisplayClass19_0.<Run>b__0()
   at Stride.Games.WindowsMessageLoop.Run(Control form, RenderCallback renderCallback, Boolean useApplicationDoEvents)
   at Stride.Games.GameWindowWinforms.Run()
   at Stride.Games.GamePlatform.Run(GameContext gameContext)
   at Stride.Games.GameBase.Run(GameContext gameContext)
   at Program.<Main>$(String[] args) in I:\Crafter\Crafter.Windows\CrafterApp.cs:line 2

[MechWarrior99]

This was something we discussed to some length before in a PR (closed now for a number of reasons). Not sure if this exact issue was brought up, but similar potential other ones were. And at the time, there was push back on the change.

I still fully agree it should be moved out. Just mentioning it for more potential context and future readers. Past Conversation.

[Eideren]

Who's the dumbass who pushed back against this ...
Oh, wait, that's me, I'm the dumbass !