Skip to content

Conversation

@iche033
Copy link
Contributor

@iche033 iche033 commented Sep 8, 2025

🎉 New feature

Related to gazebosim/jetty_demo#3

Summary

Adds a new Image::ChannelData function to help extract a single channel (r, g, b, or a) from an RGB[A] image.

Thanks for profiling data from @azeey, it was found when loading the Jetty demo world a big chunk of time is spend on splitting the metallic and roughness maps of glb meshes. The main cause of slow down is found to be the Image::Pixel function as the AssimpLoader iterates through every pixel in the image. With the new Image::ChannelData function, the AssimpLoader now has a faster and more efficient way of retrieving different channels of the input image.

Note that the perf issue with Image::Pixel function was also noticed in #699 (comment). Some changes were made to speed it up but seems like it's still not efficient. We'll need to revisit this later on.

For the Jetty demo world, the speed up for splitting the channels of an image was found to be >10x. See example timings (seconds):

  • Distribution_Warehouse_Ceiling.001_MetallicRoughness
    • before: 1.22474
    • after: 0.113241
  • Distribution_Warehouse_Container.003_MetallicRoughness
    • before: 0.301955
    • after: 0.0263694
  • Distribution_Warehouse_Column.001_MetallicRoughness
    • before: 0.304193
    • after: 0.025477
  • Distribution_Warehouse_Door.001_MetallicRoughness
    • before: 0.323877
    • after: 0.0263867

The load time for the demo world reduced from 42s (with gazebosim/gz-sim#3070) to 30s on my machine.

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
  • Was GenAI used to generate this PR? If so, make sure to add "Generated-by" to your commits. (See this policy for more info.)

Generated-by: Remove this if GenAI was not used.

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by and Generated-by messages.

@iche033 iche033 requested a review from marcoag as a code owner September 8, 2025 23:17
@github-actions github-actions bot added the 🪵 jetty Gazebo Jetty label Sep 8, 2025
@iche033 iche033 requested a review from azeey September 8, 2025 23:17
@iche033 iche033 changed the title Add function for extracting a single channel from an RGB[A] image Add function for extracting single channel data from an RGB[A] image Sep 8, 2025
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks like there are some test failures.

Comment on lines 588 to 602
for (unsigned int y = 0; y < height; ++y)
{
for (unsigned int x = 0; x < width; ++x)
{
// RGBA so 4 bytes per pixel, alpha fully opaque
unsigned int colorB = metalnessData8bit[y * width + x];
unsigned int colorG = roughnessData8bit[y * width + x];
auto baseIndex = bytesPerPixel * (y * width + x);
auto color = _img.Pixel(x, (height - y - 1));
metalnessData[baseIndex] = color.B() * 255.0;
metalnessData[baseIndex + 1] = color.B() * 255.0;
metalnessData[baseIndex + 2] = color.B() * 255.0;
metalnessData[baseIndex] = colorB;
metalnessData[baseIndex + 1] = colorB;
metalnessData[baseIndex + 2] = colorB;
metalnessData[baseIndex + 3] = 255;
roughnessData[baseIndex] = color.G() * 255.0;
roughnessData[baseIndex + 1] = color.G() * 255.0;
roughnessData[baseIndex + 2] = color.G() * 255.0;
roughnessData[baseIndex] = colorG;
roughnessData[baseIndex + 1] = colorG;
roughnessData[baseIndex + 2] = colorG;
roughnessData[baseIndex + 3] = 255;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we also had a Image::SetChannelData function, we can use that instead of this nested for loop and avoid unnecessary memory allocations for the temporary std::vectors. Not sure how feasible that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saw that there's a FreeImage_SetChannel for helping to do this. But it requires converting the data back to FIBITMAP so not sure if it's more efficient. For now, I ticketed an issue for this #708

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, let's investigate this some other time then. I raised this just because I noticed the creation of the std::vectors was a significant part of the cost of this function, so I was hoping we would directly write to the Image object we create below.

@github-project-automation github-project-automation bot moved this from Inbox to In review in Core development Sep 9, 2025
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
@iche033
Copy link
Contributor Author

iche033 commented Sep 10, 2025

The windows test failures look unrelated, likely a CI machine issue

The macOS github action build failure seems to be affected by actions/runner-images#12912

Given that the jenkins homebrew build is green, I'll merge this PR.

@iche033 iche033 merged commit e481234 into gz-common7 Sep 10, 2025
12 of 14 checks passed
@iche033 iche033 deleted the iche033/split_channel branch September 10, 2025 22:47
@github-project-automation github-project-automation bot moved this from In review to Done in Core development Sep 10, 2025
@iche033
Copy link
Contributor Author

iche033 commented Sep 10, 2025

@Mergifyio backport gz-common6 gz-common5

@mergify
Copy link
Contributor

mergify bot commented Sep 10, 2025

backport gz-common6 gz-common5

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Sep 10, 2025
mergify bot pushed a commit that referenced this pull request Sep 10, 2025
…706)

Signed-off-by: Ian Chen <[email protected]>
(cherry picked from commit e481234)

# Conflicts:
#	graphics/src/AssimpLoader.cc
iche033 added a commit that referenced this pull request Sep 10, 2025
iche033 added a commit that referenced this pull request Sep 10, 2025
…706)

Signed-off-by: Ian Chen <[email protected]>
(cherry picked from commit e481234)
Signed-off-by: Ian Chen <[email protected]>

# Conflicts:
#	graphics/src/AssimpLoader.cc
iche033 added a commit that referenced this pull request Sep 11, 2025
…706)

Signed-off-by: Ian Chen <[email protected]>
(cherry picked from commit e481234)
Signed-off-by: Ian Chen <[email protected]>

# Conflicts:
#	graphics/src/AssimpLoader.cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🪵 jetty Gazebo Jetty

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants