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

[Metal] update custom_shaders_uniforms example #554

Merged

Conversation

srmainwaring
Copy link
Contributor

@srmainwaring srmainwaring commented Feb 4, 2022

This PR adds metal shaders to the custom_shaders_uniforms example.

🎉 New feature

Enhances #520

Summary

  • Metal vertex and fragment shaders are added to the custom_shaders_uniforms example.
  • Ogre2Material is modified to specify the render system being used when creating the GPU programs.
  • Add an accessor for the graphics API to Ogre2RenderEngine

Test it

Run the example using ogre2 and specifying metal as the graphics API:

./custom_shaders_uniforms ogre2 metal

custom-shaders-metal

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@srmainwaring srmainwaring marked this pull request as draft February 4, 2022 17:59
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Feb 4, 2022
@srmainwaring
Copy link
Contributor Author

@iche033 this PR is an intermediate step to getting the waves example working as it doesn't depend on textures.

Main design input I'd like is the best way to select the graphics API being used in Ogre2Material.cpp.

- Add metal shaders to the custom_shaders_uniforms example
- Set reflection pair hint for Metal pixel shaders to ensure params are set

Select materials shader language depending on graphics API

- Add a GraphicsAPI property to the private data of Ogre2Material
- Switch the language used for the GPU program depending on the API
- Set the default API to OpenGL.
- To do: missing a method to set the graphics API when creating a material

Signed-off-by: Rhys Mainwaring <[email protected]>
@srmainwaring srmainwaring force-pushed the pr/metal-custom-shaders-example branch from 7f1c4b0 to cf94549 Compare February 4, 2022 18:47
- Remove trailing whitespace

Signed-off-by: Rhys Mainwaring <[email protected]>
@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Merging #554 (ed6f256) into main (1e711f6) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #554      +/-   ##
==========================================
- Coverage   53.18%   53.14%   -0.05%     
==========================================
  Files         213      213              
  Lines       21200    21216      +16     
==========================================
  Hits        11276    11276              
- Misses       9924     9940      +16     
Impacted Files Coverage Δ
...lude/ignition/rendering/ogre2/Ogre2RenderEngine.hh 100.00% <ø> (ø)
ogre2/src/Ogre2Material.cc 64.06% <0.00%> (-1.68%) ⬇️
ogre2/src/Ogre2RenderEngine.cc 78.36% <0.00%> (-0.35%) ⬇️

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 1e711f6...ed6f256. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Feb 4, 2022

Main design input I'd like is the best way to select the graphics API being used in Ogre2Material.cpp.

how about adding an accessor function to get the graphicsAPI var from Ogre2RenderEngine?

@srmainwaring srmainwaring mentioned this pull request Feb 4, 2022
7 tasks
@srmainwaring
Copy link
Contributor Author

how about adding an accessor function to get the graphicsAPI var from Ogre2RenderEngine?

Yes that works for me, the variable is already available in the private data. I'll have to check if Ogre2Material has access to the render engine as well.

@iche033
Copy link
Contributor

iche033 commented Feb 4, 2022

I think you should be able to get the engine by doing:

auto engine = Ogre2RenderEngine::Instance();

- Add an accessor to the Ogre2RenderEngine to retrieve the graphics API
- Use the new interface to select the shader language in Ogre2Material

Signed-off-by: Rhys Mainwaring <[email protected]>
@srmainwaring srmainwaring marked this pull request as ready for review February 5, 2022 00:54
@iche033
Copy link
Contributor

iche033 commented Feb 7, 2022

There is a compile error in CI due to conflict between enum and function name. This fixes the issue for me:

patch
diff --git a/ogre2/include/ignition/rendering/ogre2/Ogre2RenderEngine.hh b/ogre2/include/ignition/rendering/ogre2/Ogre2RenderEngine.hh
index c1ee131b..a1a69867 100644
--- a/ogre2/include/ignition/rendering/ogre2/Ogre2RenderEngine.hh
+++ b/ogre2/include/ignition/rendering/ogre2/Ogre2RenderEngine.hh
@@ -216,7 +216,7 @@ namespace ignition
 
       /// \brief Get the render engine's graphics API
       /// \return The graphics API enum class
-      public: GraphicsAPI GraphicsAPI() const;
+      public: rendering::GraphicsAPI GraphicsAPI() const;
 
       /// \brief Pointer to the ogre's overlay system
       private: Ogre::v1::OverlaySystem *ogreOverlaySystem = nullptr;

- Resolve enum / function name conflict

Signed-off-by: Rhys Mainwaring <[email protected]>
@srmainwaring
Copy link
Contributor Author

Updated.

@srmainwaring srmainwaring mentioned this pull request Feb 7, 2022
8 tasks
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good to me!

@iche033 iche033 merged commit 9c8eb2b into gazebosim:main Feb 7, 2022
@srmainwaring srmainwaring deleted the pr/metal-custom-shaders-example branch February 7, 2022 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants