UI input refactor #2474
UI input refactor #2474
Conversation
Added a UIDocument class that acts much the same way, but also can store state input state, removing the need to manually create it in the UISystem. Also will allow the UIRenderFeature and UISystem to be completely independent later on after Batch is removed from the UISystem.
|
Are there any things to look out for with the consolidation of mouse and touch inputs? Im personally happy to have one generic pointer event handler as I dont think the actual events between mouse and touch change much. |
Actually it was already consolidated as pointer input, but was refered to as touch, and the touch routed event didn't contain info about the actual pointer. So really, it is just a name change and additional data. |
|
Be aware that renaming makes it a breaking change, so this branch cannot be merged until we reach the next major update of Stride. |
What's the rationale? It is not explained in this PR. Is there an issue where this was discussed? |
Ahh, sorry I forgot to go over that. No issue for this, but I'll open(s) for future work. There are a couple of reasons.
So this change means that all input is handled in the same class, i nthe same update method, along with any sort of picking related logic and utilities. |
Yup, thanks for the clarification. I made sure to mark the checkbox noting the breaking changes. And it is fine, there are other breaking changes to the UI system I want to make later on anyway, so best to do them all at once anyway. I will open issues for it/them a bit later to get a discussion on them going, once I have a more fleshed out plan. |
|
Ok so no particular reason to move the code just a feeling. Well there is good reason to do the picking in the rendering loop, that's because it involves the graphics api to do so. Unless there is a good reason to change that behavior, I'd prefer we don't touch it. As the saying goes, if I ain't broke don't fix it. Update loop and draw loop might run at different rate, so picking during the update loop could be incorrect and involve issues with multi threading. @xen2 what's your take on this? To me this PR is a good example of why it is better to discuss it with the team (either by opening an issue or on our Discord server) before starting to do any work on your own. Final note: don't get me wrong, I'm all about refactoring if that makes the code cleaner. But we need to make sure it doesn't introduce any regression and is correct in term of architecture. |
|
@Kryptos-FR @MechWarrior99 did discuss this PR with us, see https://discord.com/channels/500285081265635328/500290856532574209/1283586888409550868 I did mention over there that picking inside of Draw would be more correct though, but let's check the changes before dismissing this wholesale. |
|
Hmm, I wouldn't say there isn't a good reason to move it @Kryptos-FR. Mind you I am not sure if the draw and update run at different rates (think they do?). But if the draw loop does run faster than the update loop (where the input events are populated) then it could raise the same event multiple times, which would be a issue for pointer pressed and released events. And if the draw does run slower, it could completely miss inputs, as the input events are cleared at the start of each update by the InputManager. This also means that all pointer events are raised on their UIElements in the draw loop, so any code that responds to those events will also run in the draw loop. I probably should have said this more explcitly in the PR from the start! As for graphics usage, while it does use the graphics API, in the end it actually is only the viewport (used to unproject a screen point to world space using a matrix, to get a ray). And the size of the dispaly (used for getting the world view projection matrix of a UIComonent's UI). To me these feel pretty minor compared to the amount of input related code, and for the most part do not require the current render state, so don't need to be in the draw loop. (Full discloure, I am not completely sure that the way I am getting the viewport is the best in this PR, but from looking around the code base it seemed to be the most correct way to do.) Looks like Eideren beat me to it. But yeah I did actually ask on discord before starting work, perhapse I should have linked it before, that's my bad. Maybe it is better to make a Issue for these things so they are a little easier to notice and not get hidden? I do totally get not wanting to touch code that is already working fine, as it can introduce bugs and regressions as you say. But I think not refactoring old code to be easier to work with hampers the code quality and accessability of the project. If after review if its decided that this isn't the way to go about to, so be it. |
sources/editor/Stride.Assets.Presentation/AssetEditors/UIEditor/Adorners/SizingAdorner.cs
Outdated
Show resolved
Hide resolved
| @@ -847,25 +847,19 @@ protected override void OnPreviewTouchMove(TouchEventArgs args) | |||
| args.Handled = true; | |||
| } | |||
|
|
|||
| private static void RaiseLeaveTouchEventToHierarchyChildren(UIElement parent, TouchEventArgs args) | |||
| private static void RaiseLeavePointerEventToHierarchyChildren(UIElement parent, PointerEventArgs args) | |||
There was a problem hiding this comment.
Maybe RaisePointerLeaveEventInHierarchy to match similar calls' naming scheme.
| public PointerEventArgs Clone() | ||
| { | ||
| return new PointerEventArgs() | ||
| { | ||
| Device = Device, | ||
| PointerId = PointerId, | ||
| Position = Position, | ||
| DeltaPosition = DeltaPosition, | ||
| DeltaTime = DeltaTime, | ||
| EventType = EventType, | ||
| IsDown = IsDown, | ||
| WorldPosition = WorldPosition, | ||
| WorldDeltaPosition = WorldDeltaPosition | ||
| }; | ||
| } |
There was a problem hiding this comment.
Would it make sense to make this class a record ? Cloning would be automatically generated and would enable a couple of pattern matching operations
There was a problem hiding this comment.
It inherits from RoutedEventArgs which inherits from System.EventArgs, so that would need to change. I haven't read up on all the details of record yet so can't say for certain if it makes sense and what the knock-on side effects would be. But from what I remember, I think it could, if we are fine with not using System.EventArgs
| { | ||
| if (renderUIElement.Page?.RootElement == null) | ||
| if (renderContext == null || sceneSystem == null || sceneSystem.GraphicsCompositor == null) |
There was a problem hiding this comment.
Shouldn't those checks be the callers' responsibility, or passed by the caller, I feel like the method should do what its name implies, if the caller calls it in an invalid state then it should error out instead of silently failing.
Not too keen on the rest of the method either, the editor workaround is too fragile, and multi camera setup won't work appropriately because of the shared renderContext and Viewport0. You have to move logic relying on those to the render loops
There was a problem hiding this comment.
Shouldn't those checks be the callers' responsibility, or passed by the caller
Not sure, it is a private method involving the values of the class that is calling it. And if you called it else where you would want the same checks, no? I was also copying the style of UpdateKeyEvents() in UISystem which has the checks in itself as well.
the editor workaround is too fragile, and multi camera setup won't work appropriately because of the shared renderContext and Viewport0.
I actually agree with both of these. However, to me this feels like an issue of either me not knowing the correct way to get the required info, or the engine not exposing the info in a way that can be used. This is especially the case for Viewport, because as I was looking through the code to try to understand how it works and is used, there are lots of places the mention/refer to supporting multiple viewports, but not actually support it. Not sure if using Viewport is even needed, but I haven't been able to figure out exactly how and why Viewport does.
See my reply to UIRenderFeature for my thinking.
There was a problem hiding this comment.
Most of that would come from inheriting from RenderFeature and overriding RenderFeature.Draw, which is called by the forward renderer, scene renderer and such.
The data you require there is setup by those systems right before calling draw. You can get your viewport from context.CommandList.Viewport for example.
You might be able to write a system to preemptively gather active viewports before draw, retrieve the scene system and such to take care of all of that in Update but it'll very likely look like an even larger hack than it already is
There was a problem hiding this comment.
Instead of preemptively gathering them, would probably be simple to cache a list of viewports from the last draw. Either just for the UI, or could make it more generic for the rendering system so in the future other systems could use them. I do feel like there is possibility of other gameplay/interactive use for the viewports.
There was a problem hiding this comment.
That's kind of a hack, not sure what kind of side effects this would create given that those would always be one frame behind Update()
There was a problem hiding this comment.
I can see how it could be a hack. I think of it more as using the most recently fully rendered data. The only rendering related data that is used is the GraphicsDevice.Presenter.BackBuffer's size, and the Viewport. Unless the viewports change size or location there will be no idfference as they are otherwise constant. If there is a change, the only possible side effect would be the pointer position being incorrect until the next draw.
The ForceAspectRationSceneRenderer.cs is the only place I can see that updates a Viewport in a way that would impact it.
The chance of the a pointer position being off by normally a small amount for a frame seems pretty tiny to me. And worth the benefits to me.
(Of course there could be other side effects I don't see. But the code is pretty straight forward seeming to me as to what is used when. But I could be missing something!)
| var uiPointerEvent = new PointerEventArgs() | ||
| { | ||
| Action = TouchAction.Down, | ||
| Timestamp = time, | ||
| ScreenPosition = currentTouchPosition, | ||
| ScreenTranslation = pointerEvent.DeltaPosition, | ||
| Device = pointerEvent.Device, | ||
| PointerId = pointerEvent.PointerId, | ||
| Position = pointerEvent.Position, | ||
| DeltaPosition = pointerEvent.DeltaPosition, | ||
| DeltaTime = pointerEvent.DeltaTime, | ||
| EventType = pointerEvent.EventType, | ||
| IsDown = pointerEvent.IsDown, | ||
| WorldPosition = intersectionPoint, | ||
| WorldTranslation = intersectionPoint - state.LastIntersectionPoint | ||
| WorldDeltaPosition = intersectionPoint - uiDocument.LastIntersectionPoint | ||
| }; |
There was a problem hiding this comment.
record's clone with feature might work well to clarify this logic here
| { | ||
| if (input == null || !input.HasMouse) | ||
| if (input == null || !input.HasMouse) // no input for thumbnails |
There was a problem hiding this comment.
Same deal, shouldn't this be the caller's responsibility ? Or maybe create a Try proxy for this call
|
|
||
| if (renderContext == null) | ||
| renderContext = RenderContext.GetShared(Services); | ||
|
|
||
| UpdatePointerInput(); |
There was a problem hiding this comment.
See my other comment in UpdatePointerInput
Was used by TouchEventArgs. But that has been refactored to PointerEventArgs and now uses the action time directly from the pointer input event.
Its functionality is already covered by IsPointerDown defined in UIElement. Tested at runtime to ensure there is no discrepancy in the behavior when using IsPointerDown.
PR Details
This refactor focuses on the input handling of the Runtime UI system, making it easier to understand, and more consistant.
UIRenderFeatureand in toUISystem, this means it now happens in the Update loop instead of the Draw loop.TouchandClicktoPointer.PointerEventArgs(formerlyTouchEventArgs), to give more control, and in the future allow for UIElements to be able to 'capture' a pointer's input (to allow for better drag actions).UIDocumentthat acts as a container for a UI's state and info required for it. Along with a EntityProcessor forUIComponentsto create these and pass them to theUISystem.Additional Notes
I am not sure about the naming of
UIDocument, doesn't quite feel the most descriptive. And the term 'document' feels like it could be used better else where later on perhapse. Maybe in the editor to refer to assets which can be opened, or maybe some other file format. Nothing else was coming to me in the moment though.Types of changes
Checklist