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

Updated to work with ign-plugin #171

Closed
wants to merge 1 commit into from
Closed

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Nov 3, 2020

This PR depends and needs a release from this other PR gazebosim/gz-plugin#30

All the symbols are loaded in the executable because of the RTLD_GLOBAL. This logic will filter the symbols

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

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from iche033 as a code owner November 3, 2020 15:58
@ahcorde ahcorde self-assigned this Nov 3, 2020
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Nov 3, 2020
@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #171 (56250d6) into main (e34fa79) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #171   +/-   ##
=======================================
  Coverage   52.07%   52.07%           
=======================================
  Files         143      143           
  Lines       13105    13108    +3     
=======================================
+ Hits         6824     6826    +2     
- Misses       6281     6282    +1     
Impacted Files Coverage Δ
src/RenderEngineManager.cc 76.59% <100.00%> (+0.37%) ⬆️
...e/ignition/rendering/base/BaseGaussianNoisePass.hh 96.66% <0.00%> (-3.34%) ⬇️

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 e34fa79...56250d6. Read the comment docs.

@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Nov 3, 2020
@chapulina
Copy link
Contributor

Let me just check if I understood why this PR is needed. If gazebosim/gz-plugin#30 gets in, ign-rendering won't work properly anymore unless this filtering is done?

This sounds like a breaking change on ign-plugin, that will required a major version bump on that library.

I'm also curious about what other pluginNames are showing up here that don't contain rendering. Do you remember?

@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 1, 2021

Yes, this may require some more mayor version bumps: ign-gazebo, ign-gui, ign-rendering, sensors.

I'm also curious about what other pluginNames are showing up here that don't contain rendering. Do you remember?

I will check it.

@chapulina
Copy link
Contributor

With the proposal on gazebosim/gz-plugin#35, I think this PR isn't necessary.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021
@chapulina
Copy link
Contributor

Since #236, we're looking explicitly for plugins that implement the ignition::rendering::RenderEnginePlugin interface, and in case we find several, we load the first one and print a warning to the user.

So I'm closing this one because I think it is no longer necessary. Please speak up if you think otherwise, @ahcorde .

@chapulina chapulina closed this Mar 19, 2021
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 🏢 edifice Ignition Edifice needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants