-
Notifications
You must be signed in to change notification settings - Fork 270
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
Load and run visual plugin (system) on GUI side #1275
Conversation
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
#include <sdf/Element.hh> | ||
#include <sdf/parser.hh> |
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.
alphabetize
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.
hmm I think this means alphabetize and also take into account case-sensitivity? If so, done in 9f4a753
#include <set> | ||
|
||
#include "../../GuiRunner.hh" |
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.
is it fine to add relative paths ?
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 think so. It's was done here as well: https://github.com/ignitionrobotics/ign-gazebo/blob/ign-gazebo6/src/gui/plugins/modules/EntityContextMenu.cc#L18
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-gazebo6 #1275 +/- ##
===============================================
- Coverage 62.25% 62.06% -0.20%
===============================================
Files 278 278
Lines 23203 23315 +112
===============================================
+ Hits 14445 14470 +25
- Misses 8758 8845 +87
Continue to review full report at Codecov.
|
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, just some minor style issues
Signed-off-by: Ian Chen <[email protected]>
@iche033 - just checked and that this works for Metal with gazebosim/gz-rendering#554 🙂 Here are the metal shaders: deform_fs.metal.zip This commit adds textures: srmainwaring@66a8490, then we can run the waves example waves-gazebo.mov |
* load and run visual plugins on gui end Signed-off-by: Ian Chen <[email protected]> * scene update event emitted on both server and gui side Signed-off-by: Ian Chen <[email protected]> * shader param update working Signed-off-by: Ian Chen <[email protected]> * sim time, constants, full example working Signed-off-by: Ian Chen <[email protected]> * add integration test Signed-off-by: Ian Chen <[email protected]> * code cleanup Signed-off-by: Ian Chen <[email protected]> * more code cleanup Signed-off-by: Ian Chen <[email protected]> * style fixes and add some comments Signed-off-by: Ian Chen <[email protected]> * review changes Signed-off-by: Ian Chen <[email protected]> * require display for shader param test Signed-off-by: Ian Chen <[email protected]> * style and comment Signed-off-by: Ian Chen <[email protected]>
oh nice, looks like you forward ported this to Garden? I can update the shader param system to read metal shaders, e.g. something like:
This just lets user specify multiple shader files and the system will then select the best shader to use depending on the platform. How does that look? |
Implemented the above idea in the shader_param_metal branch. I'll create a PR once I can upload metal shaders to Fuel. I've also created gazebosim/gz-rendering#558 to accomodate cases like gazebosim/gz-rendering#554 where extra params ( |
Yes, just cherry-picked it in. Most of the Metal support needs Garden now because of various ABI breaking interface changes. Did you want me to post a PR for the texture add-on or will you pull the commit into the shader_param_metal branch? Either way is good for me. Your proposed changes for multi-shader language support and extra param checks look good! Appreciate everyone is super busy, would you mind asking the other reviewers for the Metal GUI PR if they could take a look? It would be nice to get it merged while it's passing CI clean and before it needs a rebase. |
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1 |
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/using-a-custom-hlms-with-ignition-rendering/1315/2 |
🎉 New feature
Related PR: gazebosim/gz-rendering#533 - Needed for running the
shader_param.sdf
demo world added in this PR, and also forshader_param_system
integration test to pass.shader_param_system
integration test now passesSummary
This PR allows visual plugins to be loaded on the gui side.
Changes include:
VisualPlugin
component to store the<plugin>
sdf elementVisualPlugin
gui event forGzSceneManager
to notifyGuiRunner
that a new visual plugin is detectedGuiRunner
to load and run systems (visual plugin)SimulationRunner
SceneUpdate
event that systems can connect to on both the server and gui side for renderingShaderParam
visual plugin (ported from gazebo-classic) to test the whole pipelineTest it
Run the
shader_param.sdf
world - this world contains a sphere model with the newShaderParam
system that modifies its appearance using custom shaders. Requires gazebosim/gz-rendering#533Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.