Fix drawing slider with touch inserts a random control point at beginning #30263
Conversation
| if (CurrentPlacement == null) | ||
| return; |
There was a problem hiding this comment.
I assume this was added specifically for the new usage of this in Update()? Can this conditional be moved there?
There was a problem hiding this comment.
In OnMouseMove(), but yeah sure, applied in 40b98d4.
| updatePlacementTimeAndPosition(); | ||
| } | ||
|
|
||
| protected override bool OnMouseMove(MouseMoveEvent e) |
There was a problem hiding this comment.
Did you measure what the performance overhead of this is, if any? Given input is polled at 1000hz I expect this will be a bit redundant / slower.
There was a problem hiding this comment.
As per conversation in discord (https://discord.com/channels/188630481301012481/188630652340404224/1295374677585170575) it's probably fine
There was a problem hiding this comment.
What are the changes in this file for?
There was a problem hiding this comment.
Quoting from OP:
PlacementBlueprintTestSceneis also updated to reflect that fix, with a test case in slider placement blueprint test showing the same issue.
Although I should mention that the reflection here is a bit different than ComposeBlueprintContainer in that there's an auxiliary drawable being added to the scene and used for reading mouse movement events.
The reason for that is that the tested placement blueprint is under a manual input manager, and the PlacementBlueprintTestScene is outside of the manual input manager, so I cannot directly override OnMouseMove on the test scene level, and instead need a drawable inside of the input manager hierarchy.
Before I go on explaining how this PR tackles on the issue, some might find it easier to just read the diff and say it makes more sense how this PR does it, but failing that, the explanation follows.
Placement blueprints rely on
SnapResults to determine the position at which the hit object should be placed, rather than directly reading the mouse position from the input manager.The blueprint container holding the placement blueprint (
ComposeBlueprintContainer) handles updating theSnapResultof the blueprint, specifically atUpdate().This is working well for desktop/non-touch users, since there's always a time gap between the user moving their cursor to a specific point and clicking on that point to initiate a placement. However, for touch input, the user taps on a specific point, sending a mouse-move and mouse-down event in a single frame.
Since input events are propagated at the containing input manager's
Update(), then the blueprint handles the mouse-down event with a staleSnapResultstate from the previous frame (because we're currently inInputManager.Update(), we haven't got toComposeBlueprintContainer.Update()for the snap result to be updated with the mouse position of the touch tap yet).Now with all of this being said, this PR solves things by having
ComposeBlueprintContainerhandle mouse-move events and update the snap result of the blueprint, right before the blueprint receives the subsequent mouse-down event.PlacementBlueprintTestSceneis also updated to reflect that fix, with a test case in slider placement blueprint test showing the same issue.ScreenRecording_10-13-2024.11-16-11_1.MP4
(note that the first drag not placing a slider and causing a selection instead is expected, it will be fixed in a PR directly after this. I split it to ease the review overhead).