Add API for getting Image bytes as std::vector

[luca-della-vedova]

🎉 New feature

Closes #369

Summary

This PR adds a new API for common::Image to get the data as an std::vector<unsigned char> and deprecates the API where the user is expected to pass a pointer, a length and free the memory manually.

There is a fair bit of code duplication, I was trying to come up with a way to reuse some code but it would have probably made deprecation harder in the future.
For example I thought of having the std::vector version of the function just copy the raw array returned by the Data functions but that would have required refactoring when they are removed. In alternative I could have the old Data functions just allocate a C array and copy the std::vector's data, but now their performance would get quite a bit worse (need two allocations instead of one)
"Thanks to" the code duplication all the deprecated Data / DataImpl functions can be safely deleted and no visible behavior change should be exposed to the users.

Test it

All the unit tests with the old API have been converted to use the new API and are still successful. I also added a new test case for the deprecated functions to make sure the behavior between the old and the new ones is exactly the same (all the bytes match). The deprecated tests have deprecation warning silencing to avoid cluttering the build output and are in a separate test case so they can be easily removed in the future.

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

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.

[chapulina]

There's a new deprecation warning, we should update ImageHeightmap to use the new API.

/Users/jenkins/workspace/ignition_common-ci-pr_any-homebrew-amd64/ign-common/geospatial/src/ImageHeightmap.cc:63:13: warning: 'Data' is deprecated [-Wdeprecated-declarations]
  this->img.Data(&data, count);
            ^

[luca-della-vedova]

There's a new deprecation warning, we should update ImageHeightmap to use the new API.

/Users/jenkins/workspace/ignition_common-ci-pr_any-homebrew-amd64/ign-common/geospatial/src/ImageHeightmap.cc:63:13: warning: 'Data' is deprecated [-Wdeprecated-declarations]
  this->img.Data(&data, count);
            ^

Done thanks!
Happy to see that this is a case where the new API indeed prevents a memory leak, if the heightmap was in an unsupported format we would return the function early and delete[] wouldn't be called, leaking the memory

[codecov]

Codecov Report

Merging #372 (f13b5ba) into main (17e2c6a) will increase coverage by 0.08%.
The diff coverage is 95.55%.

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   77.85%   77.93%   +0.08%     
==========================================
  Files          85       85              
  Lines       10783    10823      +40     
==========================================
+ Hits         8395     8435      +40     
  Misses       2388     2388              

Impacted Files	Coverage Δ
graphics/include/gz/common/Image.hh	97.05% <ø> (ø)
geospatial/src/ImageHeightmap.cc	58.13% <66.66%> (-1.87%)	⬇️
graphics/src/Image.cc	76.56% <97.61%> (+3.54%)	⬆️

---

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 17e2c6a...f13b5ba. Read the comment docs.

[mjcarroll]

I'm happy with this, but I think we should fix usage up the stack before merging to avoid introducing a whole bunch of deprecation warnings.

I would propose fixing those first, or if you are constrained for time, removing the deprecations until we get cycles to fix the warnings.

[luca-della-vedova]

I'm happy with this, but I think we should fix usage up the stack before merging to avoid introducing a whole bunch of deprecation warnings.

I would propose fixing those first, or if you are constrained for time, removing the deprecations until we get cycles to fix the warnings.

Agreed, that's pretty trivial and I actually prepared all the branches a few days ago, I didn't open the PRs to avoid making too much noise until this was in a "good enough state".
Sounds like we reached that point so here they are:

gz-gui
gz-sim

[mjcarroll]

I'm happy with this once CI looks good.

We can cut a release and get the downstream PRs unblocked then.

[mjcarroll]

Only failure is a known flaky test on MacOS.

[mjcarroll]

Getting this in for nightlies so that others can merge tomorrow am.