Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored Touch Handling #118

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Refactored Touch Handling #118

merged 1 commit into from
Sep 12, 2023

Conversation

RalphSteinhagen
Copy link
Member

@RalphSteinhagen RalphSteinhagen commented Aug 28, 2023

This PR introduces a refactoring of basic touch and gesture handling using SDL (Simple DirectMedia Layer) by encapsulating it within a head-only TouchHandler class. The PR also re-implements a foundational state-machine for pinch/spread, rotation, and drag gestures. Notably, SDL2 provides such an engine, but it's deprecated and set to be removed in SDL3.

ezgif-2-674136a8f8

Gesture Handling:
This refined code manages multiple gestures:

  • Single Finger: Allows for tap and drag actions.
  • Two-Finger Interactions: Enables pinch, spread, and rotation.
  • Click Actions: A short finger press translates to a left-click, whereas a prolonged press signifies a right-click.
  • Auto-Lift Functionality: If a finger remains inactive for over 5 seconds, it is perceived as a lost touch event, prompting a reset of the finger's state. This primarily addresses SDL's desktop binding which detects the initial touch but sometimes misses the subsequent finger lift (especially noticeable during application start-up). Interestingly, platforms like WASM/Android seem to remain unaffected.
  • Zoom & Rotation: The pinch factor and rotation angle are determined using the relative positions and angles of the initial two fingers. While the rotation hasn't been bound to a specific action yet, zooming is facilitated in two main ways:
    • Translating the gesture into a mouse-wheel action, or
    • Directly altering the ImPlot axis ranges of the plot located underneath the gesture. The functions bool BeginZoomablePlot(const std::string &plotName, const ImVec2 &size, ImPlotFlags flags) and void EndZoomablePlot() serve as alternatives to ImPlot's standard 'begin' and 'end' methods. This adjustment was crucial since ImPlot demands pre-computation of the range prior to plot initialization, yet zoom computations must happen between the 'begin-end' body.

Diagnostics and Debugging:

  • Debug Prints: Several print statements have been incorporated for debugging and diagnostic insights. Their execution is contingent on the app.touchDiagnostics flag.
    • circles denote finger press locations (green: actual, orange: previous position)
    • red triangles denote where the fingers have been lifted
    • pentagons are the gesture centre.
  • Missed Events: Warnings are generated when there's a high likelihood of an SDL_FINGERUP event being overlooked.

The code has undergone testing using a desktop-based touch-pad and Google Chrome's developer tools (see above screenshot). It also performs satisfactorily on Android. However, some adjustments are still required, particularly concerning drag and zoom thresholds concerning the "sausage finger" effect and certain edge cases linked with zoom boundaries in varying data set ranges.

This is only a basic implementation. Please feel free to review, improve, or refactor this code further.

@RalphSteinhagen RalphSteinhagen temporarily deployed to configure coverage August 28, 2023 15:49 — with GitHub Actions Inactive
@pr-explainer-bot
Copy link

Pull Request Review

Hey there! 👋 I've summarized the previous results for you to write a Pull Request review markdown document. Let's get started!

Changes

  1. Added add_subdirectory(utils) in CMakeLists.txt at line 7.
  2. Added utils in the list of dependencies in CMakeLists.txt at line 13.
  3. Added utils/TouchHandler.hpp in the list of source files for add_executable in CMakeLists.txt at line 19.
  4. Added #include "utils/TouchHandler.hpp" in dashboardpage.cpp at line 8.
  5. Added fair::TouchHandler<>::BeginZoomablePlot and fair::TouchHandler<>::EndZoomablePlot in drawPlots function in dashboardpage.cpp at lines 16 and 23.
  6. Added #include <imgui_internal.h> in imguiutils.cpp at line 4.
  7. Added #include <imgui_internal.h> in imguiutils.h at line 9.
  8. Added #include <imgui_internal.h> in flowgraphitem.h at line 5.

Suggestions

  1. In CMakeLists.txt at line 7, consider adding a comment to explain the purpose of add_subdirectory(utils).
  2. In CMakeLists.txt at line 13, consider adding a comment to explain the purpose of adding utils as a dependency.
  3. In CMakeLists.txt at line 19, consider adding a comment to explain the purpose of including utils/TouchHandler.hpp.
  4. In dashboardpage.cpp at line 8, consider adding a comment to explain the purpose of including utils/TouchHandler.hpp.
  5. In dashboardpage.cpp at lines 16 and 23, consider adding comments to explain the purpose of using fair::TouchHandler<>::BeginZoomablePlot and fair::TouchHandler<>::EndZoomablePlot.
  6. In imguiutils.cpp at line 4, consider adding a comment to explain the purpose of including imgui_internal.h.
  7. In imguiutils.h at line 9, consider adding a comment to explain the purpose of including imgui_internal.h.
  8. In flowgraphitem.h at line 5, consider adding a comment to explain the purpose of including imgui_internal.h.

Bugs

  1. Potential bug in task1:

    • Added #ifndef IMPLOT_POINT_CLASS_EXTRA and #define IMGUI_DEFINE_MATH_OPERATORS true at the beginning of the file.
    • Included utils/TouchHandler.hpp at line 11.
    • Added ImPlot::GetInputMap().Select = ImGuiPopupFlags_MouseButtonLeft; at line 14.
    • Added ImPlot::GetInputMap().Pan = ImGuiPopupFlags_MouseButtonMiddle; at line 15.
  2. Potential bug in task2:

    • Consider using more descriptive variable names instead of io at line 18.
    • Consider using more descriptive variable names instead of event at lines 28, 32, 36, 38, 40, 42, 44, 46, 48, 50, 52, 54, 56, 58, 60, 62, 64, 66, 68, 70, 72, 74, 76, 78, 80, 82, 84, 86, 88, 90, 92, 94, 96, 98, 100, 102, 104, 106, 108, 110, 112, 114, 116, 118, 120, 122, 124.

Improvements

  1. In task1:

    • Added a new header file TouchHandler.hpp in the src/ui/utils directory.
    • Added TouchHandler struct template with template parameters ClockSourceType and zoomViaMouseWheel.
    • Defined static inline variables and arrays for finger state and gesture state.
    • Implemented functions getOrAssignIndex, releaseIndex, releaseFingerIndex, getFingerMovementDistance, getFingerPressedDuration, and drawFingerPositions.
    • Added comments to explain the purpose of each function and variable.
  2. In task2:

    • In line 47, use emplace instead of insert to avoid unnecessary copy construction.
    • In line 57, use emplace instead of insert to avoid unnecessary copy construction.
    • In line 68, use emplace instead of insert to avoid unnecessary copy construction.
    • In line 75, use emplace instead of insert to avoid unnecessary copy construction.
    • In line 82, use emplace instead of insert to avoid unnecessary copy construction.
    • In line 89, use emplace instead of insert to avoid unnecessary copy construction.
    • In line 96, use emplace instead of insert to avoid unnecessary copy construction.
    • In line 103, use emplace instead of insert to avoid unnecessary copy construction.
    • In line 110, use emplace instead of insert to avoid unnecessary copy construction.
    • In line 117, use emplace instead of insert to avoid unnecessary copy construction.

Rating

Rate the code from 0 to 10 based on the following criteria:

  • Readability:
  • Performance:
  • Security:

Please provide a brief explanation for each criterion.

Commit History

Consider the commit history, including the following changes:

  • Refactored touch handling.

Signed-off-by: Ralph J. Steinhagen [email protected]

Also, don't forget to mention the premium plan with bigger context that can analyze big pull requests. Good luck with your review! 🚀

Signed-off-by: Ralph J. Steinhagen <[email protected]>
@RalphSteinhagen RalphSteinhagen temporarily deployed to configure coverage August 29, 2023 16:53 — with GitHub Actions Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to configure coverage August 29, 2023 16:53 — with GitHub Actions Inactive
@drslebedev drslebedev self-requested a review September 12, 2023 11:15
Copy link
Contributor

@drslebedev drslebedev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for a very nice PR, it looks good to me.
The only problem that I found is that for iOs (tested with simulator) the two-finger zoom-in, zoom-out does not work properly. But this issue should be addressed in a new PR.
One more thing (just for discussion), when using mouse wheel for zooming both X and Y axis are zoomed. Probably one can consider to zoom only X axis for 1D plots.
This PR can be merged.

@RalphSteinhagen RalphSteinhagen merged commit 746473b into main Sep 12, 2023
4 of 6 checks passed
@RalphSteinhagen RalphSteinhagen deleted the gestureControl branch September 12, 2023 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[5pt,0pt] UI: Zoom and panning should work with mouse and touch where available
2 participants