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

Testing Windows fix #392

Merged
merged 6 commits into from
Oct 17, 2023
Merged

Testing Windows fix #392

merged 6 commits into from
Oct 17, 2023

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Oct 10, 2023

🦟 Bug fix

Summary

Update:

Updated CMake to build camera related sensor components with the CameraSensorUtil.cc src file.

Added GZ_SENSORS_VISIBLE to fix the windows build issue described in #391 (comment).

The CameraSensorUtil.hh header file is in the src directory so we're not actually installing that header or exposing the functions. Happy to try an alternative solution if there's a better one.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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.

Signed-off-by: Ian Chen <[email protected]>
@iche033 iche033 marked this pull request as draft October 10, 2023 21:01
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #392 (696cf97) into main (e52f97c) will not change coverage.
The diff coverage is 50.00%.

❗ Current head 696cf97 differs from pull request most recent head 0fcf479. Consider uploading reports for the commit 0fcf479 to get more accurate results

@@           Coverage Diff           @@
##             main     #392   +/-   ##
=======================================
  Coverage   72.34%   72.34%           
=======================================
  Files          37       37           
  Lines        5052     5052           
=======================================
  Hits         3655     3655           
  Misses       1397     1397           
Files Coverage Δ
src/CameraSensor.cc 75.49% <100.00%> (ø)
src/DepthCameraSensor.cc 70.90% <0.00%> (ø)

Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
@iche033 iche033 marked this pull request as ready for review October 11, 2023 04:16
double _intrinsicsCx, double _intrinsicsCy,
double _intrinsicsS, double _clipNear,
double _clipFar);
math::Matrix4d GZ_SENSORS_VISIBLE buildProjectionMatrix(
Copy link
Contributor

@azeey azeey Oct 11, 2023

Choose a reason for hiding this comment

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

I think it would be better to compile CameraSensorUtil.cc with each of the components that need it rather than making it visible here since it now becomes part of our public ABI.

Edit: Technically, I think it is visible by default on linux, so it is part of the public ABI (unfortunately) already, but when gazebosim/gz-cmake#166 is resolved, this would break in Linux as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

testing alternative fix in d5b64ef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Windows CI looks good with the new changes.

@iche033 iche033 merged commit 19a88e8 into main Oct 17, 2023
5 of 6 checks passed
@iche033 iche033 deleted the win_fix branch October 17, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants