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

Add a method to render targets and cameras to retrieve a Metal texture #478

Merged

Conversation

srmainwaring
Copy link
Contributor

🎉 New feature

Replaces #477 which targeted the wrong branch.

Relates to: #461 and discussions in #422. It accompanies PR #463 but does not depend upon it.

Summary

This PR provides access to the underlying Metal texture object. The functions MetalId() and RenderTextureMetalId()
provide the Metal equivalent of GLId() and RenderTextureGLId() for OpenGL.

The functions expect a parameter void* _texturePtr which is the address of a pointer to void* as the object required for Metal is an Objective-C type id<MTLTexture> which we do not want exposed in the interface (the type is the id for an object
that implements the MTLTexture protocol).

Note: the functions do not add a dependency on Objective-C to the ignition rendering libraries, but they are intended to be used in Objective-C code as follows:

void *textureIdPtr = nullptr;
texture->RenderTextureMetalId(&textureIdPtr);
id<MTLTexture> metalTexture = CFBridgingRelease(textureIdPtr);

The translation unit containing the code should be compiled with ARC enabled and will need to link against the AppKit and Metal frameworks. When using cmake this is accomplished by CMakeLists.txt entries such as:

target_link_libraries(simple_demo_qml_metal PUBLIC
    ${IGNITION-RENDERING_LIBRARIES}
    ${OGRE2_LIBRARIES}
    ${OPENGL_LIBRARIES}
    Qt5::Core
    Qt5::Gui
    Qt5::Qml
    Qt5::Quick
    "-framework AppKit"
    "-framework Metal"
)

# Enable ARC on selected source files
set_source_files_properties(
    IgnitionRenderer.mm
    RenderThread.mm
    ThreadRenderer.mm
    PROPERTIES
    COMPILE_FLAGS
        "-fobjc-arc"
)

There is a runtime dependency on an upstream change to ogre-next: OGRECave/ogre-next@3b11873. Note that this PR does not require the upstream change to build as the Ogre interface used, getCustomAttribute, is available on the base class Ogre::TextureGpu and is used by the OpenGL functions.

Test it

These changes have no impact on existing functionality using the OpenGL render system.

To test these functions requires a version of ogre-next v2-2 that includes the change OGRECave/ogre-next@3b11873.

There is work in progress to enable the use of Metal for the example simple_demo_qml and for ign-gui and ign-gazebo. They depend upon this PR and provide a means to test the changes:

https://github.com/srmainwaring/ign-rendering/tree/feature/ign-rendering6-metal-simple_demo_qml
https://github.com/srmainwaring/ign-gui/tree/feature/ign-gui7-metal
https://github.com/srmainwaring/ign-gazebo/tree/feature/ign-gazebo7-metal

For more detail see comment: #463 (comment)

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
  • Some tests fail on macOS:
89% tests passed, 11 tests failed out of 102

Total Test time (real) =  34.16 sec

The following tests FAILED:
	 19 - UNIT_Heightmap_TEST (SEGFAULT)
	 59 - UNIT_RenderingIface_TEST (SEGFAULT)
	 75 - UNIT_Utils_TEST (Failed)
	 83 - INTEGRATION_depth_camera (SEGFAULT)
	 85 - INTEGRATION_camera (Failed)
	 87 - INTEGRATION_render_pass (SEGFAULT)
	 89 - INTEGRATION_shadows (Failed)
	 91 - INTEGRATION_scene (Failed)
	 93 - INTEGRATION_segmentation_camera (Failed)
	 95 - INTEGRATION_sky (Failed)
	 97 - INTEGRATION_thermal_camera (Failed)

The SEGFAULTS occur on the tear down of tests using ogre (when deleting Ogre::Root) and the failed tests are because macOS does not have OpenGL support for ogre2.

Note to maintainers: Remember to use Squash-Merge

…tal texture id

- These functions provide the Metal equivalent of GLId() and RenderTextureGLId() for OpenGL.
- The argument is the address of a pointer to void* as the object required for Metal is an objective-c type id<MTLTexture> which we do not want exposed in the interface
- There is a runtime dependency on an upstream change to ogre-next: OGRECave/ogre-next@3b11873

Signed-off-by: Rhys Mainwaring <[email protected]>
- Fix an inconsistent parameter name causing a CI failure.

Signed-off-by: Rhys Mainwaring <[email protected]>
- Fix an unused parameter warning causing a CI failure.

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

codecov bot commented Oct 21, 2021

Codecov Report

Merging #478 (020f09a) into main (63264a9) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #478      +/-   ##
==========================================
- Coverage   53.49%   53.43%   -0.06%     
==========================================
  Files         192      192              
  Lines       19566    19587      +21     
==========================================
+ Hits        10466    10467       +1     
- Misses       9100     9120      +20     
Impacted Files Coverage Δ
include/ignition/rendering/Camera.hh 100.00% <ø> (ø)
include/ignition/rendering/RenderTarget.hh 66.66% <ø> (ø)
include/ignition/rendering/base/BaseCamera.hh 65.20% <0.00%> (-0.97%) ⬇️
...nclude/ignition/rendering/base/BaseRenderTarget.hh 60.22% <0.00%> (-1.41%) ⬇️
ogre2/src/Ogre2Camera.cc 70.30% <0.00%> (-3.12%) ⬇️
ogre2/src/Ogre2RenderTarget.cc 84.43% <0.00%> (-1.63%) ⬇️
...e/ignition/rendering/base/BaseGaussianNoisePass.hh 100.00% <0.00%> (+3.33%) ⬆️

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 63264a9...020f09a. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Oct 21, 2021

Changes look good to me. We'll need to update our ogre 2.2 formula then.

Let's get this in main first.

I'll think of a workaround to get the metal texture id for Fortress.

@iche033
Copy link
Contributor

iche033 commented Oct 21, 2021

@darksylinc are there any changes you're planning to make in the ogre-next v2-2 branch in the near future? If not, I'll just bump our bottle to be at the latest commit (currently OGRECave/ogre-next@312bf40) from the v2-2 branch

@darksylinc
Copy link
Contributor

Nothing major. 2.2 is in bugfixes stage.

I am evaluating to see what/if we can backport some of the new VCT stuff to 2.2 but I cannot say for certain right now; and it's not worth waiting for that.

@iche033
Copy link
Contributor

iche033 commented Oct 21, 2021

alright sounds good!

@iche033 iche033 merged commit 8ea86c9 into gazebosim:main Nov 1, 2021
@srmainwaring srmainwaring deleted the feature/ign-rendering-metal-texture branch November 1, 2021 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants