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

Sort out MinimalScene's mouse events #295

Merged
merged 10 commits into from
Oct 1, 2021
Merged

Sort out MinimalScene's mouse events #295

merged 10 commits into from
Oct 1, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Sep 28, 2021

🦟 Bug fix

Fixes #293

Summary

I started working on the issues pointed out on #293 and ran into various other issues and inconsistencies about the way we're handling mouse events. Currently there are scrolls and drags being sent as left clicks, both mouse presses and releases broadcasted as clicks... I'm still sorting it out, hopefully will be done in time for the release.

Summary of changes:

  • Add 3 new events: drag, scroll and press
  • Press with any button is used to pick the orbit target
  • Drag information was previously being acquired from a combination of the previous event and a left click. Now it's passed through the drag event.
  • I think scroll info was also coming from other events, but now it's its own event
  • The HandleMouseViewControl was a confusing function that re-sent the left clicks, so I removed it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added bug Something isn't working beta Targeting beta release of upcoming collection labels Sep 28, 2021
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Sep 28, 2021
@ahcorde
Copy link
Contributor

ahcorde commented Sep 30, 2021

This PR fixes:

  • Scroll zoom moves camera in wrong direction
  • Context menu triggered on mouse release after a zoom action

I'm trying to debug -> Incorrect mouse anchor point.

ahcorde and others added 3 commits September 30, 2021 17:04
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina marked this pull request as ready for review October 1, 2021 00:09
@chapulina chapulina changed the title Start sorting out MinimalScene's mouse events Sort out MinimalScene's mouse events Oct 1, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
…s/ign-gui into chapulina/6/events

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@codecov
Copy link

codecov bot commented Oct 1, 2021

Codecov Report

Merging #295 (bb14e9a) into main (653b0fb) will increase coverage by 0.11%.
The diff coverage is 46.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #295      +/-   ##
==========================================
+ Coverage   65.31%   65.42%   +0.11%     
==========================================
  Files          32       32              
  Lines        4621     4686      +65     
==========================================
+ Hits         3018     3066      +48     
- Misses       1603     1620      +17     
Impacted Files Coverage Δ
include/ignition/gui/GuiEvents.hh 100.00% <ø> (ø)
...interactive_view_control/InteractiveViewControl.cc 16.52% <10.71%> (+2.24%) ⬆️
src/plugins/minimal_scene/MinimalScene.cc 58.92% <32.75%> (-0.54%) ⬇️
src/Conversions.cc 95.00% <78.94%> (-5.00%) ⬇️
src/GuiEvents.cc 100.00% <100.00%> (ø)
src/plugins/scene3d/Scene3D.cc 48.83% <100.00%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac58762...bb14e9a. Read the comment docs.

Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

This looks good to me, some minor comments/questions

include/ignition/gui/GuiEvents.hh Outdated Show resolved Hide resolved
include/ignition/gui/GuiEvents.hh Outdated Show resolved Hide resolved
include/ignition/gui/GuiEvents.hh Outdated Show resolved Hide resolved
src/GuiEvents.cc Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
chapulina and others added 2 commits September 30, 2021 19:14
Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina enabled auto-merge (squash) October 1, 2021 02:19
@chapulina chapulina merged commit c337047 into main Oct 1, 2021
@chapulina chapulina deleted the chapulina/6/events branch October 1, 2021 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection bug Something isn't working 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants