Fix native FlyByCamera touch gestures and input regression coverage - #2981
toaster0123 wants to merge 2 commits into
Conversation
Supersedes jMonkeyEngine#2943, retaining its native touch mapping and rotation math while addressing pointer ownership, mouse emulation, lifecycle resets, and input-pipeline regression coverage. Based on commits 61bacec and 84f4232 from the original PR. Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: riccardobl <4943530+riccardobl@users.noreply.github.com>
| case MOVE: | ||
| if (touchPointerId == -1 || event.getPointerId() != touchPointerId | ||
| || (inputManager != null && inputManager.isSimulateMouse())) { | ||
| return; | ||
| } | ||
| break; |
There was a problem hiding this comment.
A zero-delta MOVE still falls through to two rotateCamera calls, and each one ends in cam.setAxes(...) → onFrameChange(), which rebuilds all six world frustum planes, the view matrix, and the view-projection matrix. Android delivers batched MOVE events every frame while a pointer is down, so a finger that just rests on the screen (or rests while a virtual joystick is in use) pays that cost ~120 times a second for no visible change. A cheap guard keeps the behaviour identical and the existing zeroDeltaDoesNotRotateOrReleasePointer test still holds.
| case MOVE: | |
| if (touchPointerId == -1 || event.getPointerId() != touchPointerId | |
| || (inputManager != null && inputManager.isSimulateMouse())) { | |
| return; | |
| } | |
| break; | |
| case MOVE: | |
| if (touchPointerId == -1 || event.getPointerId() != touchPointerId | |
| || (inputManager != null && inputManager.isSimulateMouse())) { | |
| return; | |
| } | |
| if (event.getDeltaX() == 0f && event.getDeltaY() == 0f) { | |
| return; | |
| } | |
| break; |
There was a problem hiding this comment.
Addressed: MOVE now returns immediately when both deltas are zero, after the pointer/emulation checks, without clearing pointer ownership. I strengthened the queued-input regression to count Camera.onFrameChange() calls: three idle moves (including signed zero) trigger no rebuilds, and a subsequent nonzero move from the same pointer still rotates. The strengthened test fails on the previous implementation with six unnecessary rebuilds and passes with the guard. All 35 FlyByCamera cases and the full core build pass. Please re-review the updated patch.
There was a problem hiding this comment.
Confirmed — the guard landed exactly where it should: after the touchPointerId / isSimulateMouse() checks, so pointer ownership is untouched by a zero-delta MOVE and the drag keeps working. Signed zero is covered too (-0f == 0f is true in Java).
The strengthened test is the part I like most: counting onFrameChange() calls makes this a real performance regression rather than just "rotation didn't change", and it still asserts the follow-up nonzero move rotates from the same pointer. Good call.
There was a problem hiding this comment.
Verified the guard in the current patch: MOVE returns before either rotateCamera call when both deltas are zero (signed zero included, since -0f == 0f), and pointer ownership is untouched. The rebuild-counting test is a nice way to pin that down. Nothing blocking from me.
| private static final String FLYCAM_JOYSTICK_UP = "FLYCAM_JoystickUp"; | ||
| private static final String FLYCAM_JOYSTICK_DOWN = "FLYCAM_JoystickDown"; | ||
| private static final String FLYCAM_TOUCH = "FLYCAM_Touch"; | ||
| private static final float TOUCH_ROTATION_SCALE = 1f / 1024f; |
There was a problem hiding this comment.
Small maintainability note: 1f / 1024f mirrors the hard-coded 1024f in InputManager.onMouseMotionEventQueued, and keeping touch and mouse rotation in lockstep is the whole point of this change. If the engine's mouse scale is ever tweaked, this constant would silently drift out of parity and the hard-coded 1024f expectations in the test would fail with a confusing message. It would be more robust to lift the scale into a shared constant on InputManager and use it from both sides — happy to see that as a follow-up rather than blocking on it here.
There was a problem hiding this comment.
Addressed in this update with a package-private InputManager.MOUSE_AXIS_SCALE constant shared by the mouse X/Y dispatch path and native FlyByCamera touch rotation. This removes the duplicate production scale without adding public API or changing the existing power-of-two scaling. The tests intentionally retain independent numeric expectations for the current sensitivity, alongside the real mouse/touch parity cases for mixed-axis movement, inverted Y, and rotation speed, so accidental scale changes remain detectable.
There was a problem hiding this comment.
Checked, and this is the right shape. InputManager.MOUSE_AXIS_SCALE is package-private (no new public API), and both the mouse X/Y dispatch in onMouseMotionEventQueued and the native touch rotation in FlyByCamera read from it, so the two paths can't drift. 1f / 1024f is a compile-time constant and an exact power of two, so the multiplication is bit-identical to the old / 1024f — mouse sensitivity is unchanged. Nice follow-up anyway.
There was a problem hiding this comment.
Nice — InputManager.MOUSE_AXIS_SCALE is used by both onMouseMotionEventQueued (X and Y) and the native touch path, and keeping it package-private keeps it out of the public API. Good call on keeping the hard-coded 1024f expectations in the tests so an accidental scale change still shows up as a clear failure.
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Nice work on the pointer lifecycle — the first-unconsumed-DOWN latch, the fresh-DOWN-only rule, and the isSimulateMouse() de-duplication all look right to me, and I traced the yaw/pitch math against assertDirection and it checks out.
- The test suite is the highlight: driving the real
InputManagerqueue instead of pokingonTouch()directly is what makes theconsumed-pointer andisSimulateMousecases actually meaningful, and the touch-vs-mouse parity test is exactly the kind of regression net this needed. - One small fix: bail out of
MOVEwhen both deltas are zero, otherwise a resting finger triggers two full camera matrix rebuilds per frame (inline comment has the patch). - Non-blocking:
TOUCH_ROTATION_SCALEduplicates the1024finInputManager; sharing one constant would keep the parity guarantee from drifting.
Once the zero-delta guard lands I think this is good to go.
Address both review comments on PR jMonkeyEngine#2981. Preserve pointer ownership for zero-delta moves and test that no camera frame rebuild occurs before the next real drag. Keep touch and mouse scaling in one package-private InputManager constant while retaining independent numeric regression expectations.
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Both follow-ups are addressed and the patch reads well now.
- The zero-delta
MOVEguard sits after the pointer/emulation checks, so it skips the rotation without dropping the active pointer — exactly the behaviour the previous version got wrong. TheonFrameChange()counting in the test turns it into a genuine regression guard, and it still proves a resting finger doesn't permanently kill the drag. InputManager.MOUSE_AXIS_SCALEis package-private, shared by the mouse dispatch and the touch path, and1f / 1024fis an exact power of two, so mouse sensitivity is bit-for-bit unchanged. Keeping literal1024fexpectations in the tests still catches accidental scale changes.
The pointer lifecycle (first unconsumed DOWN, fresh-DOWN-only release rule, reset on disable/unregister) and the InputManager-driven tests all still check out on re-read. Nice work — approved from me.
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Both points from the last round are addressed in the current patch:
MOVEnow bails out on a zero delta (signed zero included) before eitherrotateCameracall, so a resting finger no longer triggers a full frustum/view/view-projection rebuild every frame, and the active pointer survives. The rebuild-counting test pins this down.InputManager.MOUSE_AXIS_SCALEis shared by the mouse X/Y dispatch path and the native touch rotation, package-private so no new public API, with the tests keeping independent numeric expectations as a tripwire.
Pointer ownership, the fresh-DOWN rule, isSimulateMouse de-duplication, and the touch-vs-mouse parity coverage all look good to me. Approved — nice work.
2 participants
Summary
Supersedes #2943 and builds on its native-touch implementation by Copilot and @riccardobl, addressing the review at #2943 (review).
FlyByCameraand latch the first unconsumedDOWN, including nonzero pointer IDs when another control has consumed an earlier pointer.UP, and require a freshDOWNrather than inheriting a remaining finger. Clear gesture ownership on disable/unregister.InputManagerevent pipeline with queued touch and mouse events instead of invokingFlyByCamera.onTouch()directly.Related: #2929. The original PR remains open and unchanged.
Verification
javac --release 8 -Xlint:unchecked -Xlint:-options -Werrorcompilation of all core main/plugin/tool sources passes; the same 35 cases also pass under JUnit Platform 6.1.3 directly.:jme3-core:buildpasses after a clean rebuild: 518 test cases, 0 failures, 0 errors, 1 skipped; includes core Javadoc, jars, and configured Checkstyle tasks.git diff --checkpasses for this change.