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

Emit key events from Scene3D #224

Merged
merged 8 commits into from
Jun 30, 2021
Merged

Emit key events from Scene3D #224

merged 8 commits into from
Jun 30, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented May 25, 2021

Signed-off-by: ahcorde ahcorde@gmail.com

🎉 New feature

Summary

This PR builds on top of this other PR #213. This PR makes the 3D scene emit key events (press and release), that can be used by other plugins.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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

@ahcorde ahcorde added the enhancement New feature or request label May 25, 2021
@ahcorde ahcorde requested a review from scpeters May 25, 2021 19:43
@ahcorde ahcorde requested a review from chapulina as a code owner May 25, 2021 19:43
@ahcorde ahcorde self-assigned this May 25, 2021
@ahcorde ahcorde mentioned this pull request May 26, 2021
8 tasks
Base automatically changed from chapulina/3/events to ign-gui3 May 27, 2021 09:44
@ahcorde ahcorde force-pushed the ahcorde/3/keyevents branch from 60be577 to 9d42483 Compare May 27, 2021 09:58
@codecov
Copy link

codecov bot commented May 27, 2021

Codecov Report

Merging #224 (d2d545c) into main (2fc2862) will increase coverage by 1.58%.
The diff coverage is 100.00%.

❗ Current head d2d545c differs from pull request most recent head dd6edfa. Consider uploading reports for the commit dd6edfa to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #224      +/-   ##
==========================================
+ Coverage   65.56%   67.14%   +1.58%     
==========================================
  Files          25       25              
  Lines        3104     3138      +34     
==========================================
+ Hits         2035     2107      +72     
+ Misses       1069     1031      -38     
Impacted Files Coverage Δ
include/ignition/gui/GuiEvents.hh 100.00% <ø> (ø)
src/GuiEvents.cc 100.00% <100.00%> (ø)
src/plugins/scene3d/Scene3D.cc 48.69% <100.00%> (+6.46%) ⬆️

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 2fc2862...dd6edfa. Read the comment docs.

include/ignition/gui/GuiEvents.hh Show resolved Hide resolved
src/plugins/scene3d/Scene3D.cc Outdated Show resolved Hide resolved
@ahcorde ahcorde requested a review from mjcarroll May 27, 2021 15:45
include/ignition/gui/GuiEvents.hh Outdated Show resolved Hide resolved
src/plugins/scene3d/Scene3D.hh Outdated Show resolved Hide resolved
@ahcorde
Copy link
Contributor Author

ahcorde commented May 27, 2021

Do not merge this PR yet, i will add key modifiers such us control, shift or alt.

@ahcorde
Copy link
Contributor Author

ahcorde commented May 27, 2021

I think we should add the same modifiers to the Mouse clicks, what do you think @scpeters ?

@ahcorde ahcorde requested a review from scpeters May 27, 2021 18:57
@ahcorde
Copy link
Contributor Author

ahcorde commented May 28, 2021

@scpeters and @mjcarroll this is ready for another pass

@mjcarroll
Copy link
Contributor

Looks like something has changed on focal, presumably because of Qt5?

  /github/workspace/test/integration/scene3d.cc:271: Failure
  Expected equality of these values:
    Qt::Key_A
      Which is: 65
    keyPressedValue
      Which is: 16777251

@ahcorde ahcorde mentioned this pull request Jun 8, 2021
8 tasks
@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 9, 2021

@mjcarroll I think it's flaky...

@mjcarroll
Copy link
Contributor

I think adding the copy constructor messed with all of the derived classes here as well.

@chapulina chapulina mentioned this pull request Jun 14, 2021
8 tasks
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

The idea of the PR is good, we just need to fix the build:

   In file included from /github/workspace/src/GuiEvents_TEST.cc:21:
  /github/workspace/include/ignition/gui/GuiEvents.hh: In constructor 'ignition::gui::events::KeyRelease::KeyRelease(const ignition::common::KeyEvent&)':
  /github/workspace/include/ignition/gui/GuiEvents.hh:269:38: error: use of deleted function 'ignition::common::KeyEvent::KeyEvent(const ignition::common::KeyEvent&)'
               : QEvent(kType), key(_key)

src/plugins/scene3d/Scene3D.cc Outdated Show resolved Hide resolved
src/plugins/scene3d/Scene3D.hh Outdated Show resolved Hide resolved
src/plugins/scene3d/Scene3D.hh Outdated Show resolved Hide resolved
@ahcorde ahcorde added the needs upstream release Blocked by a release of an upstream library label Jun 15, 2021
@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 15, 2021

@chapulina PR is in gazebosim/gz-common#224

We need a ign-common3 release.

@ahcorde ahcorde requested a review from chapulina June 15, 2021 08:04
@ahcorde ahcorde requested a review from chapulina June 16, 2021 15:02
@chapulina chapulina added the 🏰 citadel Ignition Citadel label Jun 17, 2021
@chapulina chapulina mentioned this pull request Jun 17, 2021
7 tasks
@mjcarroll mjcarroll requested a review from jennuine as a code owner June 21, 2021 15:27
@mjcarroll mjcarroll removed the needs upstream release Blocked by a release of an upstream library label Jun 21, 2021
@chapulina
Copy link
Contributor

Same as #228, since this is not used by any Citadel plugins yet, let's target it at main.

My concern with these events is that they're not PIMPL'ed and once merged into a stable version we can't iterate on them (see #239 ).

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/3/keyevents branch from df861fa to d708418 Compare June 25, 2021 10:18
@ahcorde ahcorde changed the base branch from ign-gui3 to main June 25, 2021 10:19
@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 25, 2021

@chapulina retargeted to main and add PIMPL pattern

ahcorde added 3 commits June 25, 2021 12:23
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina chapulina added 🏯 fortress Ignition Fortress and removed 🏰 citadel Ignition Citadel labels Jun 25, 2021
ahcorde and others added 2 commits June 25, 2021 20:10
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Pushed tweaks on cdcf459.

LGTM with happy CI.

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

ahcorde commented Jun 28, 2021

Merging, some know tests are failing: UNIT_WorldStats_TEST and UNIT_Application_TEST

@ahcorde ahcorde enabled auto-merge (squash) June 28, 2021 12:11
@ahcorde ahcorde requested a review from chapulina June 28, 2021 13:44
@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 28, 2021

@osrf-jenkins retest this please

1 similar comment
@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 29, 2021

@osrf-jenkins retest this please

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde merged commit a9ddfad into main Jun 30, 2021
@ahcorde ahcorde deleted the ahcorde/3/keyevents branch June 30, 2021 18:38
@chapulina chapulina mentioned this pull request Jul 14, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants