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 more events from Scene3D #213

Merged
merged 17 commits into from
May 27, 2021
Merged

Emit more events from Scene3D #213

merged 17 commits into from
May 27, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Apr 27, 2021

🎉 New feature

Closes #209

Related to gazebosim/gz-rviz#70

Summary

This PR makes the 3D scene emit more events, such as clicks and hover, that can be used by other plugins. Most of the code here has been ported from ignition::gazebo::Scene3D.

I got stuck writing tests for it though, for some reason the render engine plugin is not properly loaded. I spent some time digging deep into ign-plugin with no luck. I'll leave this as draft until I can address this.

Test it

I'd like to add an example plugin that makes use of these events, so others can use it as a pattern to copy for their needs.

Checklist

  • Signed all commits for DCO
  • Added tests - that's the tough part
  • Added example and/or tutorial - TODO
  • 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

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

@chapulina chapulina marked this pull request as ready for review April 29, 2021 18:27
@chapulina
Copy link
Contributor Author

I removed the tests that weren't working and ticketed #216. Opening this for review.

It's worth reading the conversation on #209. The current approach to emitting events is necessary for some use cases, but adds unnecessary complexity for others. I think we could revisit the current architecture in a future release in favor of an objectName-based approach for specific use cases.

@scpeters scpeters self-requested a review April 29, 2021 18:39
There seems to be a problem with loading the ignition-rendering-ogre
plugin from the Scene3D test if it links to that plugin. Making
Scene3D_TEST.cc into an integration test works because it doesn't
directly call any plugin methods.

This also changes the linking for the Grid3D plugin to only link
to the ignition-rendering core library target instead of the
IGNITION-RENDERING_LIBRARIES variable which includes the ogre
component library plugins.

Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Member

scpeters commented May 8, 2021

I've fixed the issue with loading the render engines documented in #216 and have merged those changes into this branch. I moved Scene3D_TEST.cc to test/integration, and it seems to be working now.

@chapulina
Copy link
Contributor Author

I added a test for the render event in 23c0578. I won't have time to add tests for the other events for a while. Options:

  1. Get this in with the current tests and add more tests later (this is already more testing than most plugins)
  2. Someone else picks up this PR and adds tests for the click and hover events
  3. Wait until I come back

@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #213 (096c868) into ign-gui3 (a7e659e) will increase coverage by 6.00%.
The diff coverage is 47.05%.

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

@@             Coverage Diff              @@
##           ign-gui3     #213      +/-   ##
============================================
+ Coverage     59.55%   65.56%   +6.00%     
============================================
  Files            23       23              
  Lines          2715     2817     +102     
============================================
+ Hits           1617     1847     +230     
+ Misses         1098      970     -128     
Impacted Files Coverage Δ
src/plugins/scene3d/Scene3D.cc 42.38% <47.05%> (+29.78%) ⬆️

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 a7e659e...e75cff4. Read the comment docs.

@scpeters
Copy link
Member

I added a test for the render event in 23c0578. I won't have time to add tests for the other events for a while. Options:

  1. Get this in with the current tests and add more tests later (this is already more testing than most plugins)
  2. Someone else picks up this PR and adds tests for the click and hover events
  3. Wait until I come back

I can try adding some more tests. Do you know how to create fake mouse clicks from a test?

Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde mentioned this pull request May 25, 2021
8 tasks
Signed-off-by: ahcorde <[email protected]>
@scpeters
Copy link
Member

I just noticed the windows build has been broken since 23c0578

error C2491: 'ignition::gui::TestHelper::staticMetaObject': definition of dllimport static data member not allowed

@ahcorde ahcorde mentioned this pull request May 26, 2021
8 tasks
@scpeters
Copy link
Member

I've added some expectations about the value of the Vector3d passed with the click events in 48171dd, and if CI is reasonable, then I will be ready to approve

Comment on lines 192 to 193
std::cerr << "hoverToScene point["
<< hoverToScene->Point() << "]" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this trace ?

Copy link
Member

Choose a reason for hiding this comment

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

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
@ahcorde ahcorde enabled auto-merge (squash) May 27, 2021 09:40
@ahcorde ahcorde merged commit 4a0159b into ign-gui3 May 27, 2021
@ahcorde ahcorde deleted the chapulina/3/events branch May 27, 2021 09:44
@ahcorde ahcorde mentioned this pull request Jun 8, 2021
8 tasks
chapulina added a commit that referenced this pull request Jun 22, 2021
* Release 3.5.1 (#195)

Signed-off-by: Steve Peters <[email protected]>

* check_test_ran.py: remove grep/xsltproc (#203)

We aren't using QTest anymore in ign-gui, so remove
the parts of check_test_ran.py that translated QTest
xml files into junit, since they don't work easily
on Windows and aren't needed any longer.

Fixes #198.

Signed-off-by: Steve Peters <[email protected]>

* Fixed material specular in scene3D (#218)

Signed-off-by: ahcorde <[email protected]>

* Remove tools/code_check and update codecov (#222)

Signed-off-by: Louise Poubel <[email protected]>

* Removed duplicated code with rendering::sceneFromFirstRenderEngine (#223)

Signed-off-by: ahcorde <[email protected]>

* Emit more events from Scene3D (#213)

* Start porting events from ign-gazebo

Signed-off-by: Louise Poubel <[email protected]>

* Remove test that fails due to #216

Signed-off-by: Louise Poubel <[email protected]>

* Move Scene3d_TEST.cc to test/integration

There seems to be a problem with loading the ignition-rendering-ogre
plugin from the Scene3D test if it links to that plugin. Making
Scene3D_TEST.cc into an integration test works because it doesn't
directly call any plugin methods.

This also changes the linking for the Grid3D plugin to only link
to the ignition-rendering core library target instead of the
IGNITION-RENDERING_LIBRARIES variable which includes the ogre
component library plugins.

Signed-off-by: Steve Peters <[email protected]>

* process qt events to allow scene to initialize

Signed-off-by: Steve Peters <[email protected]>

* Add test helper to check event

Signed-off-by: Louise Poubel <[email protected]>

* added more tests

Signed-off-by: ahcorde <[email protected]>

* make linters happy

Signed-off-by: ahcorde <[email protected]>

* Move TestHelper code to .cc file, fix windows?

Signed-off-by: Steve Peters <[email protected]>

* Fix windows?

Signed-off-by: ahcorde <[email protected]>

* Fix windows?

Signed-off-by: Alejandro Hernández <[email protected]>

* Expect values of Vector3 point in click events

Signed-off-by: Steve Peters <[email protected]>

* Remove debug message

Signed-off-by: Steve Peters <[email protected]>

* Remove unused variable

Signed-off-by: Steve Peters <[email protected]>

Co-authored-by: Steve Peters <[email protected]>
Co-authored-by: ahcorde <[email protected]>

* Avoid grid3D crash (#227)

Signed-off-by: ahcorde <[email protected]>

* Confirmation dialog when closing main window (#225)

* Adds confirmation dialog when closing window
* Updates docs and extends test coverage.
* Adds dialog_on_exit atribute to example .config

Signed-off-by: Franco Cipollone <[email protected]>

* update codeowners (#232)

Signed-off-by: Jenn Nguyen <[email protected]>

* 🎈 3.6.0 (#233)

Signed-off-by: Louise Poubel <[email protected]>

* Bump required ign-rendering version to 4.8 (#234)

Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Steve Peters <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Franco Cipollone <[email protected]>
Co-authored-by: Jenn Nguyen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants