Support for textures as binary blobs

[luca-della-vedova]

🎉 New feature

Edit: Ready for review!

Summary

This PR is an early request-for-feedback on having support for in-memory textures. It reached a "working" state with the gz-common luca/inmemory_textures branch where GLB assets with textures stored as binary blobs (array of bytes containing the file) are correctly parsed and rendered. I.e. below a world with a nurse looking at an ambulance stretcher, both defined as single file GLBs with embedded textures:

Before I refine too much the implementation I'd like to know whether the API makes sense or I should define a different one, specifically:

Right now, to set a texture we currently use the SetTexture method which takes as an input a filename, however we would need to support arbitrary binary blobs in memory for formats with embedded textures to work.

I have a few ideas and currently implemented one but happy to scrap it completely:

Parse images into a gz::common::Image and change SetTexture (or add an overloaded method) to accept a gz::common::Image as an input.

This would have worked and sounded the cleanest but I was concerned about doing additional processing that might not be needed. For example, we could parse an image and change it into RGBA8 but it is unclear what format might be demanded by downstream users of gz-rendering. For example OGRE2 can parse PNGs straightaway hence if we preprocessed the image we would have unnecessarily inflated the data before copying it around. Also gz::Image doesn't expose raw data yet so we would have needed to add an API to set (from compressed) and get raw data, which is what we would have ended up using anyway.

Change SetTexture to accept a vector of raw data parameter, set to default empty

I.e. going from:

void SetTexture(const std::string &_texture);

To

void SetTexture(const std::string &_texture, std::vector<unsigned char> _data = {});

In this case _texture would either be a URI or a unique name that can be used to identify the binary texture (to be generated to make sure that if we load multiple instances of the same model through binary blobs we don't duplicate the texture).
This feels generally a lot simpler but the ambiguity of the _texture argument (URI to a file or unique name) for the two use cases felt a bit unclear.

This approach would probably reduce the diff / complexity quite a bit since we can leverage all the existing code and just add a check to the SetTexture function and load the texture from memory instead of a file if the _buf argument is not empty.

Brand new overloaded function

This is the approach I went for when prototyping for simplicity but I'm happy to scrap, it currently looks like this:

void SetTexture(const std::vector<unsigned char> &_buf, const std::string& _name)

The tricky part here is that we have to save the texture buffer and name to an internal member to make sure the data is retained when the class is copied around (which seems to happen quite a bit!)
Now it gets ambiguous, if a user calls the normal SetTexture and tells the rendering engine "load this file", then calls the overloaded method and tells the engine "load this texture from a binary file" what should the appropriate behavior be? I'm guessing clearing the previous texture, which is fine but again the API is slightly unclear, if you call a SetTexture(URI), then somewhere else a SetTexture(binary data), then next time you call the getter Texture() to get the URI the information is gone, or it's not gone but it's not rendered, just a bit tricky.
I found this especially tricky to figure out in the custom copy constructor since its result will now depend on the order of operations.

Other considerations

• std::vector<> vs pointers: I am currently using std::vector<> for the data but noticed while debugging that there are quite a few copies / clones happening behind the scenes and the data gets copied around at least twice (once in the gz-common -> gz-rendering transition and once through a Clone() call). I feel this is a non trivial amount for data that is multiple megabytes per object so happy to move to a shared_ptr approach if that would help with performance.
• Passing vs not passing image formats: The asset parsing library has knowledge of what format the data is in (i.e. png, jpg). Right now the image is literally a binary file that includes the signature that can be used to decode the format hence it "just works" with OGRE2. We could add a parameter to pass the format specifically but I couldn't find a valid case for it yet.
• We would need a similar set of SetTexture overloads for all the PBR maps like metalness, roughness, normal, and probably lightmaps, hence it would be good to know the best API to make the diff as simple / manageable as possible

@iche033 @chapulina for API feedback!

Test it

Stay tuned!

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• 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

[iche033]

Now it gets ambiguous, if a user calls the normal SetTexture and tells the rendering engine "load this file", then calls the overloaded method and tells the engine "load this texture from a binary file" what should the appropriate behavior be? I'm guessing clearing the previous texture, which is fine but again the API is slightly unclear, if you call a SetTexture(URI), then somewhere else a SetTexture(binary data), then next time you call the getter Texture() to get the URI the information is gone, or it's not gone but it's not rendered, just a bit tricky.

So based on the current approach, I would expect this:

// set texture using old method
SetTexture(tex_name);

// get texture. This returns `tex_name`
auto result = Texture();

// set texture using new method. Clears old tex data.
SetTexture(blob, new_tex_name);

// this returns `new_tex_name`
auto result2 = Texture();

I found this especially tricky to figure out in the custom copy constructor since its result will now depend on the order of operations.

Not sure why two SetTexture calls are needed there? We copy textureName and only copy textureData if it's not empty?

std::vector<> vs pointers:

I think we can assume that users can't change the texture data so we can just share them.

Passing vs not passing image formats:

If we want to support this, i think it is better to just to go with SetTexture(common/rendering::Image) since that'll handle different formats. For the current implementation, I would expect unsigned char array to be RGB8 by default and we would then have to start looking at handling different formats based on additional arg and may get complicated quickly.

Overall I'm leaning towards the SetTexture(common/rendering::Image) API which is probably more flexible. In the ogre2 implementation, the textureName var will just point to a unique name associated with that image.

[luca-della-vedova]

OK sounds good! Then I'll change the approach to use shared memory and the API to be:

SetTexture(std::string& _name, std::shared_ptr<rendering::Image> _image = nullptr);

Then current calls will go through the usual code path of using the texture as a URI and loading it from a file, if the image data is set it will be used as a unique texture name and loaded from the image raw data.
My instinct would be to use RGBA8 for a default format though, so information for textures that use alpha thresholding is also present and users can still decide to ignore it if they want to. If we don't and send RGB8 from gz::common the alpha information will be silently dropped and not retrievable by gz::rendering users

[iche033]

My instinct would be to use RGBA8 for a default format though

ok yeah that's sounds good to me

[luca-della-vedova]

Ok done! The API is here.

It not super ideal since I believe we can't change the current Texture getter since that would break downstream users (maybe could do a tick/tock cycle?):

       public: virtual std::string Texture() const = 0;

As an alternative I added another getter to get the texture data

       public: virtual std::shared_ptr<common::Image> TextureData() const = 0;

And changed the SetTexture call to take as an input a pointer to a gz::common::Image, set to nullptr by default to make sure current behavior is unaffected.

If this looks good I'll implement a similar update for PBR textures, as well as OGRE1
Edit: If OGRE1 supports it at least 😅

[iche033]

that looks good to me! I think the TextureData getter function is fine actually.

for OGRE1, you should be able to create a manual texture and load raw data to it. Here is an example: https://github.com/gazebosim/gz-rendering/blob/ign-rendering6/ogre/src/OgreDistortionPass.cc#L286-L405

[luca-della-vedova]

Thanks for the pointer! It made things a lot easier.

Still, OGRE1 doesn't really work that well, specifically I had to add a workaround to swap red and blue here. It seems that regardless of whether I specify the pixel format as PF_R8G8B8A8 or PF_B8G8R8A8 the behavior on the OGRE1 side is unchanged and the model above looks "very blue".

After the workaround the colors look better but some models (i.e. the nurse here) seem to have issues, probably due to the fact that it has an RGBA texture (so with an alpha channel)

I guess I'll focus on getting the implementation as good / clean as possible with OGRE2

[iche033]

Still, OGRE1 doesn't really work that well, specifically I had to add a workaround to swap red and blue here. It seems that regardless of whether I specify the pixel format as PF_R8G8B8A8 or PF_B8G8R8A8 the behavior on the OGRE1 side is unchanged and the model above looks "very blue".

hmm that's weird. Yeah getting OGRE2 to work first sounds good. You can ticket an issue for the OGRE1 implementation afterwards

[luca-della-vedova]

OK I think this is pretty much ready for review!

To test it you will need the dependency PRs in gz-cmake and gz-common, as well as a few assets in GLB, personally I tested with the attached ones:

GLBModels.zip

• AmbulanceStretcher is a PBR GLTF asset, under Ubuntu 20.04 and the older version of assimp 5.0.x it will only have a diffuse and normal texture, under Ubuntu 22.04 and assimp gltf PBR support it will also have metalness and roughness maps.
• StaffFemale_CheckClipboard An animated human with PBR textures, same as above for PBR support but also include animations. Also provided as a dae to make sure animations in Collada and GLB look the same.

Other assets can be found quite easily, the Khronos repo has quite a variety of them.

All assets are provided as both gltf and glb.
gltf is made of a readable json that includes the main fields that describe the mesh and points to a binary file for the binary data (i.e. vertices, normals, texture cooridnates) and image files for textures.
glb includes all the data bundled in a single binary. gltf doesn't use the API in this branch since textures are external images but glb does, if all works well they should look exactly the same.

[luca-della-vedova]

I didn't add the NormalMapData getter / copy the member variable since it seems normal maps are not implemented in OGRE1 anyway

[chapulina]

To test it you will need the dependency PRs in gazebosim/gz-cmake#278 and gazebosim/gz-common#372, as well as a few assets in GLB, personally I tested with the attached ones:

Could you add an example to the examples folder that loads that for easier testing? Bonus for an accompanying tutorial under tutorials 😄

[luca-della-vedova]

Could you add an example to the examples folder that loads that for easier testing? Bonus for an accompanying tutorial under tutorials smile

Done! Since this is all handled by the MeshManager I just added another mesh to mesh_viewer, if that makes sense.
The mesh will render differently depending on whether you are on assimp 5.0.x (it will only have a diffuse component) or assimp 5.2.x (it will have full PBR with normal, metalness and roughness maps).
Is this sufficient or were you thinking about having an example with the full API of manually setting materials? It seems for the other mesh we "just" use the output of the MeshManager without going through the trouble of creating materials manually.
It is different for ogre2_demo where there is PBR and we create the materials manually but again this is because the formats used don't support PBR natively hence we either populate it internally or cannot have it, in this case all the PBR population is handled by MeshManager and users of the library "just" have to spawn the returned mesh without changing anything.

[luca-della-vedova]

Good news! It seems the OGRE1 issue was a missing data copy caught by unit tests, now this works as intended in OGRE1 as well

[iche033]

example mesh works for me on Focal with ogre and ogre2

[iche033]

Changes look good as they are now. Just waiting for upstream PRs and green CI.

One minor note that shouldn't block this PR from getting in is if we could keep the original SetTexture / Set*Map calls and not deprecate them since this version has been the common use case till now. Now we require existing downstream users to change the call to SetTexture("tex", nullptr) where the second arg is just nullptr.

[codecov]

Codecov Report

Merging #642 (b5758f1) into main (72243ac) will increase coverage by 0.18%.
The diff coverage is 89.61%.

@@            Coverage Diff             @@
##             main     #642      +/-   ##
==========================================
+ Coverage   73.95%   74.14%   +0.18%     
==========================================
  Files         159      159              
  Lines       14243    14365     +122     
==========================================
+ Hits        10534    10651     +117     
- Misses       3709     3714       +5     

Impacted Files	Coverage Δ
include/gz/rendering/Material.hh	61.11% <66.66%> (-38.89%)	⬇️
include/gz/rendering/base/BaseMaterial.hh	71.86% <84.61%> (+0.38%)	⬆️
ogre2/src/Ogre2Material.cc	76.93% <100.00%> (+3.39%)	⬆️
ogre2/src/Ogre2Scene.cc	78.19% <100.00%> (+0.47%)	⬆️
src/base/BaseScene.cc	77.65% <100.00%> (ø)
ogre2/src/Ogre2MeshFactory.cc	80.74% <0.00%> (+1.68%)	⬆️

---

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 72243ac...b5758f1. Read the comment docs.

[chapulina]

61 Windows failures 😩 Merging 😬

[onurtore]

Tested locally, LGTM.