Skip to content

[garden] Use anti-aliasing value from <anti_aliasing> SDF element#210

Merged
iche033 merged 8 commits intogazebosim:mainfrom
WilliamLewww:wlew/anti_aliasing
Jun 14, 2022
Merged

[garden] Use anti-aliasing value from <anti_aliasing> SDF element#210
iche033 merged 8 commits intogazebosim:mainfrom
WilliamLewww:wlew/anti_aliasing

Conversation

@WilliamLewww
Copy link
Contributor

@WilliamLewww WilliamLewww commented Mar 29, 2022

🎉 [garden] Use anti-aliasing value from <anti_aliasing> SDF element

Summary

The level of MSAA is hard-coded to 2. This pull request allows users to change the MSAA level with the SDF element <anti_aliasing>.

<camera name="camera">
  <image>
    <width>640</width>
    <height>480</height>
    <format>R8G8B8</format>
    <anti_aliasing>4</anti_aliasing>
  </image>
...
</camera>

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.

Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
@WilliamLewww WilliamLewww requested a review from iche033 as a code owner March 29, 2022 22:07
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Mar 29, 2022
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I think you need to change this line

https://github.com/ignitionrobotics/ign-sensors/blob/ign-sensors6/CMakeLists.txt#L89

to

ign_find_package(sdformat12 REQUIRED VERSION 12.4)

@WilliamLewww WilliamLewww changed the title Use anti-aliasing value from <anti_aliasing> SDF element [garden] Use anti-aliasing value from <anti_aliasing> SDF element Apr 21, 2022
@chapulina chapulina requested a review from scpeters May 16, 2022 18:46
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #210 (6aa4cb9) into main (9302a7d) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #210   +/-   ##
=======================================
  Coverage   68.73%   68.73%           
=======================================
  Files          34       34           
  Lines        3419     3419           
=======================================
  Hits         2350     2350           
  Misses       1069     1069           
Impacted Files Coverage Δ
src/CameraSensor.cc 77.17% <100.00%> (ø)

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 9302a7d...6aa4cb9. Read the comment docs.

@WilliamLewww
Copy link
Contributor Author

Test failures are irrelevant to changes.

@chapulina
Copy link
Contributor

I think you need to change this line

+1 to @ahcorde 's suggestion. We should bump the required SDF version so that users know they need to upgrade when compiling, as opposed to having a compilation error due to the missing API, and needing to dig to find out what's happening.

@WilliamLewww
Copy link
Contributor Author

I think you need to change this line

+1 to @ahcorde 's suggestion. We should bump the required SDF version so that users know they need to upgrade when compiling, as opposed to having a compilation error due to the missing API, and needing to dig to find out what's happening.

Since it's currently targeting sdformat13, should I bump the minor version?

# Find SDFormat
ign_find_package(sdformat13 REQUIRED)
set(SDF_VER ${sdformat13_VERSION_MAJOR})

to

ign_find_package(sdformat13 REQUIRED VERSION 13.1)

@scpeters
Copy link
Member

scpeters commented Jun 8, 2022

I think you need to change this line

+1 to @ahcorde 's suggestion. We should bump the required SDF version so that users know they need to upgrade when compiling, as opposed to having a compilation error due to the missing API, and needing to dig to find out what's happening.

Since it's currently targeting sdformat13, should I bump the minor version?

# Find SDFormat
ign_find_package(sdformat13 REQUIRED)
set(SDF_VER ${sdformat13_VERSION_MAJOR})

to

ign_find_package(sdformat13 REQUIRED VERSION 13.1)

libsdformat13 hasn't been released yet, so we don't need to bump the minor. I think this request is out of date since it is targeting garden?

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

the change looks fine to me, though I want to point out that it does change the default behavior. I'll leave it to @iche033 to merge


// \todo(nkoeng) these parameters via sdf
this->dataPtr->camera->SetAntiAliasing(2);
this->dataPtr->camera->SetAntiAliasing(cameraSdf->AntiAliasingValue());
Copy link
Member

Choose a reason for hiding this comment

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

the default value in SDFormat is 4, so this will represent a behavior change

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm it may be worth putting a note in the migration guide about this change, otherwise changes look good to me.

Copy link
Member

Choose a reason for hiding this comment

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

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@iche033 iche033 dismissed ahcorde’s stale review June 14, 2022 00:06

comment addressed - now targeting garden

@iche033 iche033 merged commit 1dccfc4 into gazebosim:main Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🌱 garden Ignition Garden

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants