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

Non desired window opening alongside ignition GUI #436

Merged
merged 6 commits into from
Sep 30, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Sep 27, 2021

🦟 Bug fix

Summary

Fix gazebosim/gz-gui#291

Requires a PR in ign-gui6 gazebosim/gz-gui#292

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: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from iche033 September 27, 2021 22:27
@ahcorde ahcorde self-assigned this Sep 27, 2021
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Sep 27, 2021
@chapulina chapulina added beta Targeting beta release of upcoming collection bug Something isn't working labels Sep 27, 2021
@iche033
Copy link
Contributor

iche033 commented Sep 27, 2021

I think this new SetWinID API is no longer needed because winId is now passed as one of the engine params in gazebosim/gz-gui#292?

@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 28, 2021

I think this new SetWinID API is no longer needed because winId is now passed as one of the engine params in ignitionrobotics/ign-gui#292?

These changes are required because the params passed by ign-gui are parsed inside ign-rendering to offer the API to the different Render engines

Signed-off-by: ahcorde <ahcorde@gmail.com>
@codecov
Copy link

codecov bot commented Sep 28, 2021

Codecov Report

Merging #436 (4de11cb) into main (6d08ef3) will increase coverage by 0.00%.
The diff coverage is 60.00%.

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

@@           Coverage Diff           @@
##             main     #436   +/-   ##
=======================================
  Coverage   53.49%   53.49%           
=======================================
  Files         192      192           
  Lines       19549    19554    +5     
=======================================
+ Hits        10458    10461    +3     
- Misses       9091     9093    +2     
Impacted Files Coverage Δ
ogre2/src/Ogre2RenderEngine.cc 79.56% <60.00%> (-0.25%) ⬇️

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 6d08ef3...c6e547c. Read the comment docs.

@chapulina
Copy link
Contributor

These changes are required because the params passed by ign-gui are parsed inside ign-rendering to offer the API to the different Render engines

I'm not sure I understand. Can you point out where SetWinId is used?

@matosinho
Copy link

Issue fixed here for me with this branch.

1 similar comment
@matosinho
Copy link

Issue fixed here for me with this branch.

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 30, 2021

I'm not sure I understand. Can you point out where SetWinId is used?

The params variable is the same type in ign-gui and ign-rendering but both libraries need to keep it own copy.

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.

Works for me 👍

@chapulina chapulina merged commit eb7d6ea into main Sep 30, 2021
@chapulina chapulina deleted the ahcorde/fix/second_window branch September 30, 2021 18:44
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.

4 participants