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

Removed duplicated code with rendering::sceneFromFirstRenderEngine #223

Merged
merged 1 commit into from
May 26, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented May 17, 2021

Enhancement

Summary

Related to this PR in ignition-renderin gazebosim/gz-rendering#320 (comment). It uses the helper function rendering::sceneFromFirstRenderEngine to get the first scene that can be found in any rendering engine.

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

@ahcorde ahcorde added the enhancement New feature or request label May 17, 2021
@ahcorde ahcorde self-assigned this May 17, 2021
@ahcorde ahcorde requested a review from chapulina as a code owner May 17, 2021 08:19
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label May 17, 2021
@mjcarroll mjcarroll added the needs upstream release Blocked by a release of an upstream library label May 17, 2021
@mjcarroll
Copy link
Contributor

Looks like this will need a release of ign-rendering to pass CI.

@ahcorde
Copy link
Contributor Author

ahcorde commented May 26, 2021

@osrf-jenkins retest this please

@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #223 (86493e5) into ign-gui3 (890a572) will increase coverage by 0.52%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gui3     #223      +/-   ##
============================================
+ Coverage     59.03%   59.55%   +0.52%     
============================================
  Files            23       23              
  Lines          2739     2715      -24     
============================================
  Hits           1617     1617              
+ Misses         1122     1098      -24     
Impacted Files Coverage Δ
src/plugins/screenshot/Screenshot.cc 33.33% <0.00%> (+7.61%) ⬆️

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 890a572...86493e5. Read the comment docs.

@ahcorde
Copy link
Contributor Author

ahcorde commented May 26, 2021

@mjcarroll the UNIT_Application_TEST is failing buit I think it's unrelated

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.

LGTM

UNIT_Application_TEST is also failing in #225 as well as locally for me but I don't understand why?

@mjcarroll
Copy link
Contributor

I think that UNIT_Application_TEST is segfaulting at the end, which will cause it to fail.

@ahcorde
Copy link
Contributor Author

ahcorde commented May 26, 2021

In my local machine it's failing here:

auto ds = app.allWindows();

This should return only one element in the vector, but I'm getting 2.

@ahcorde
Copy link
Contributor Author

ahcorde commented May 26, 2021

As this test is unrelated to this PR I will merge it. It seems flaky

@ahcorde ahcorde merged commit a7e659e into ign-gui3 May 26, 2021
@ahcorde ahcorde deleted the ahcorde/sceneFromFirstRenderEngine branch May 26, 2021 20:17
@jennuine
Copy link
Contributor

This should return only one element in the vector, but I'm getting 2.

It fails for me there too. I think it probably is flaky

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]>
@mboisson
Copy link

mboisson commented Aug 3, 2021

This PR broke the semantic versioning for Citadel. I was trying to build Citadel with the latest versions of each package, and this yields this error:
#263

@chapulina chapulina mentioned this pull request Aug 3, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel enhancement New feature or request needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants