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

Camera: configure projection matrix from SDFormat #249

Conversation

ihasdapie
Copy link
Contributor

@ihasdapie ihasdapie commented Jul 21, 2022

🎉 New feature

Summary

This PR adds sdf fields to allow for configuration of the camera projection matrix.

Related PR in sdf9: gazebosim/sdformat#1088

Test it

Make a camera with those fields

Checklist

  • [x ] 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

@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Jul 21, 2022
Signed-off-by: Brian Chen <[email protected]>
@chapulina chapulina added the enhancement New feature or request label Jul 23, 2022
Comment on lines +108 to +113
<< " <p_fx>282</p_fx>"
<< " <p_fy>283</p_fy>"
<< " <p_cx>163</p_cx>"
<< " <p_cy>125</p_cy>"
<< " <tx>1</tx>"
<< " <ty>2</ty>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check that the parsing / setting of values was successful here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I am not mistaken this is already done in the test here:

sdf::SDFPtr sdfParsed(new sdf::SDF());
sdf::init(sdfParsed);
if (!sdf::readString(stream.str(), sdfParsed))
return sdf::ElementPtr();
return sdfParsed->Root()->GetElement("model")->GetElement("link")
->GetElement("sensor");
}

sdf::readString will return False if parsing was unsuccessful so the test should fail if the test is unsuccessful. One change that can be made is to check for the errors like this, but it's a little redundant since the return bool already tells us if it failed or not

  sdf::Errors errors;
  if (!sdf::readString(stream.str(), sdfParsed, errors)){
    ASSERT_EQ(errors.size(), 0);
    return sdf::ElementPtr();
}

Copy link
Member

Choose a reason for hiding this comment

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

this does check that it did not fail during parsing but doesn't confirm that the values are correct. I don't see a test where this is already done, so I'll see if there's an easy way to add this; if not we can merge as is

Copy link
Member

Choose a reason for hiding this comment

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

I've added some tests for the new parameters in 9c8f428

Signed-off-by: Brian Chen <[email protected]>
@scpeters
Copy link
Member

scpeters commented Sep 7, 2022

I'll make a prerelease of sdformat9 to support this ticket

@scpeters scpeters changed the title use sdf to configure camera projection matrix Camera: configure projection matrix from SDFormat Sep 7, 2022
@scpeters
Copy link
Member

scpeters commented Sep 7, 2022

I'll make a prerelease of sdformat9 to support this ticket

gazebosim/sdformat#1134

@scpeters
Copy link
Member

rerunning CI since sdformat 9.9.0 was released

@codecov
Copy link

codecov bot commented Sep 30, 2022

Codecov Report

Merging #249 (adfd9de) into ign-sensors3 (4a99c5f) will not change coverage.
The diff coverage is 100.00%.

❗ Current head adfd9de differs from pull request most recent head 69287b8. Consider uploading reports for the commit 69287b8 to get more accurate results

@@              Coverage Diff              @@
##           ign-sensors3     #249   +/-   ##
=============================================
  Coverage         79.80%   79.80%           
=============================================
  Files                23       23           
  Lines              2377     2377           
=============================================
  Hits               1897     1897           
  Misses              480      480           
Impacted Files Coverage Δ
src/CameraSensor.cc 75.37% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@scpeters scpeters removed the needs upstream release Blocked by a release of an upstream library label Sep 30, 2022
@scpeters
Copy link
Member

this is ready for review

@scpeters scpeters requested review from sanjuksha and removed request for chapulina October 5, 2022 20:21
Confirm that intrinsics and projection parameters
in CameraInfo message match the values from
SDFormat.

Signed-off-by: Steve Peters <[email protected]>
Load plugins from build folder instead of
the install prefix.

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters merged commit d090f91 into gazebosim:ign-sensors3 Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants