-
Notifications
You must be signed in to change notification settings - Fork 44
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
Added Mouse Events #228
Added Mouse Events #228
Conversation
Codecov Report
@@ Coverage Diff @@
## main #228 +/- ##
==========================================
+ Coverage 65.31% 65.56% +0.24%
==========================================
Files 24 25 +1
Lines 3088 3104 +16
==========================================
+ Hits 2017 2035 +18
+ Misses 1071 1069 -2
Continue to review full report at Codecov.
|
f7387ad
to
d249652
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but the INTEGRATION_Scene3d
test is segfaulting now.
@osrf-jenkins retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you looked into the INTEGRATION_Scene3d
test failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a PR that uses these events, @ahcorde ? It would be nice to make sure the events have everything we need before merging and releasing. These classes don't use the PIMPL pattern, so if we realize later that they need more fields, we can't break ABI. See #209 (comment) for more context.
@chapulina this PR make use of it #231 |
friendly ping @chapulina |
@ahcorde , how about we target this one at |
I will wait to this forward port #238 because it includes the scene3d tests. |
Signed-off-by: ahcorde <[email protected]>
3796b96
to
54834f3
Compare
Signed-off-by: ahcorde <[email protected]>
@chapulina retargeted to |
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
e601fd5
to
edcfb77
Compare
Signed-off-by: Louise Poubel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed some documentation and tweaks in edcfb77.
I'm on the fence about keeping both the ClickOnScene
and ClickToScene
events. I think they could be combined into a single event to avoid confusion. But I'll refrain from making more changes until @scpeters opens a PR adding the distance field as proposed in #209 (comment).
Let's go with these for now!
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: ahcorde [email protected]
🎉 New feature
Related with this issue #209. I added left and right mouse events. These events are generated by the Scene3D window, otherwise if we try to get the mouse click from other plugin we will get the click in the hole window making more difficult to handle it. Other advantage it's that we are publishing a ignition related event.
Summary
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge