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 waves example #555

Merged
merged 1 commit into from
Feb 8, 2022
Merged

Conversation

srmainwaring
Copy link
Contributor

@srmainwaring srmainwaring commented Feb 7, 2022

🎉 New feature

This PR adds metal shaders to the waves example.

Enhances #541

Summary

Test it

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

./waves ogre2 metal

waves-metal-4-textures

For further details on the implementation at see the discussion in #541

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.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Feb 7, 2022
@osrf-triage osrf-triage added this to Inbox in Core development Feb 7, 2022
@srmainwaring srmainwaring marked this pull request as draft February 7, 2022 22:37
@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #555 (e47886d) into main (9c8eb2b) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head e47886d differs from pull request most recent head 441b348. Consider uploading reports for the commit 441b348 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #555      +/-   ##
==========================================
- Coverage   53.14%   53.13%   -0.01%     
==========================================
  Files         213      213              
  Lines       21216    21218       +2     
==========================================
- Hits        11276    11275       -1     
- Misses       9940     9943       +3     
Impacted Files Coverage Δ
ogre2/src/Ogre2Material.cc 63.83% <0.00%> (-0.24%) ⬇️
...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 9c8eb2b...441b348. Read the comment docs.

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. Thanks for tidyingup other parts of the code in GlutWindow.cc.

Now that the amplitude of the waves is larger, how about moving the camera higher so it's not covered by the waves?

@srmainwaring
Copy link
Contributor Author

Now that the amplitude of the waves is larger, how about moving the camera higher so it's not covered by the waves?

Good idea, I'll do that when I rebase. Just playing around with the shader_param.sdf example - it's working with Metal so I'm part way through converting it to work with the wave shaders. I'll zip the model up and post it if it works.

Add metal shaders to the waves example
- Pass BTN by component to fragment shader
- Disable setting named constant param for textures in Ogre2Material::UpdateShaderParams
- Update camera anti-aliasing
- Increse wave amplitude and raise camera position

Fix issue with cubic textures for custom Metal shaders
- Add the same isLoaded() check applied in OgreTextureUnitState to the Type2D textures

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@srmainwaring srmainwaring marked this pull request as ready for review February 8, 2022 00:26
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!

Core development automation moved this from Inbox to In review Feb 8, 2022
@iche033 iche033 merged commit d67d20c into gazebosim:main Feb 8, 2022
Core development automation moved this from In review to Done Feb 8, 2022
@iche033 iche033 mentioned this pull request Feb 8, 2022
7 tasks
@srmainwaring srmainwaring deleted the pr/metal-waves branch February 8, 2022 02:00
@scpeters scpeters moved this from Done to Highlights in Core development Feb 14, 2022
@j-rivero j-rivero removed this from Highlights in Core development May 6, 2022
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.

None yet

2 participants