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

Add service to change camera follow pgain. #515

Closed

Conversation

bperseghetti
Copy link
Member

@bperseghetti bperseghetti commented Jan 28, 2023

🦟 Bug fix

Fixes #514

Summary

Now you can set the pgain of the follow camera when following objects. This is needed for extremely high speed objects.

better.mp4

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.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jan 28, 2023
@ahcorde
Copy link
Contributor

ahcorde commented Feb 1, 2023

@osrf-jenkins run tests please!

@ahcorde ahcorde enabled auto-merge (squash) February 1, 2023 16:28
auto-merge was automatically disabled February 3, 2023 06:33

Head branch was pushed to by a user without write access

@bperseghetti bperseghetti force-pushed the pr-set-follow-camera-pgain branch 3 times, most recently from e596e32 to 290c060 Compare February 3, 2023 17:12
@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8dae037) 72.98% compared to head (290c060) 69.18%.
Report is 8 commits behind head on gz-gui7.

❗ Current head 290c060 differs from pull request most recent head bea60bd. Consider uploading reports for the commit bea60bd to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           gz-gui7     #515      +/-   ##
===========================================
- Coverage    72.98%   69.18%   -3.80%     
===========================================
  Files           55       44      -11     
  Lines         4927     4930       +3     
===========================================
- Hits          3596     3411     -185     
- Misses        1331     1519     +188     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bperseghetti bperseghetti force-pushed the pr-set-follow-camera-pgain branch 2 times, most recently from 6150395 to ed5167d Compare February 3, 2023 20:51
examples/standalone/scene_provider/README.md Outdated Show resolved Hide resolved
examples/standalone/scene_provider/README.md Outdated Show resolved Hide resolved
src/plugins/follow_config/CMakeLists.txt Outdated Show resolved Hide resolved
src/plugins/follow_config/FollowConfig.cc Outdated Show resolved Hide resolved
src/plugins/follow_config/FollowConfig.cc Outdated Show resolved Hide resolved
src/plugins/follow_config/FollowConfig.cc Outdated Show resolved Hide resolved
src/plugins/follow_config/FollowConfig.cc Outdated Show resolved Hide resolved
src/plugins/follow_config/FollowConfig.cc Outdated Show resolved Hide resolved
src/plugins/follow_config/FollowConfig.cc Outdated Show resolved Hide resolved
test/integration/follow_config.cc Outdated Show resolved Hide resolved
@bperseghetti bperseghetti force-pushed the pr-set-follow-camera-pgain branch from c386760 to 66fbe33 Compare February 9, 2023 03:06
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

the new service calls and Follow Config plugin work well for me

@bperseghetti bperseghetti force-pushed the pr-set-follow-camera-pgain branch from 66fbe33 to c265538 Compare February 13, 2023 13:24
@bperseghetti bperseghetti force-pushed the pr-set-follow-camera-pgain branch from c265538 to 55e73fe Compare February 13, 2023 14:42

msgs::StringMsg reqName;
reqName.set_data(this->followTargetName);
node.Request(this->followTargetNameService, reqName, cbName);
Copy link
Contributor

@azeey azeey Mar 2, 2023

Choose a reason for hiding this comment

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

If you're using async service requests, you'll need to synchronize access to all the variables used by the callback since the callbacks will happen from a different thread than the Qt thread. This could help with the CI failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, just copy-capture the variable in the lambda. I think this is a lot easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will finally get back to looking at this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bperseghetti did you address the feedback ?

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@mjcarroll
Copy link
Contributor

@bperseghetti I haven't seen any movement on this one and it targets garden branches, so I'm going to remove the beta label for now.

@mjcarroll mjcarroll removed the beta Targeting beta release of upcoming collection label Aug 29, 2023
@caguero
Copy link
Contributor

caguero commented Jan 5, 2024

@bperseghetti , friendly ping. Could you review the feedback and we'll try to merge this patch?

@bperseghetti
Copy link
Member Author

@bperseghetti , friendly ping. Could you review the feedback and we'll try to merge this patch?

Yeah, will look it over again here this weekend and see if I can't make the requested changes.

Example: gz service -s /gui/follow/pgain --reqtype gz.msgs.Double --reptype gz.msgs.Boolean --timeout 2000 --req 'data: 1.0'

Signed-off-by: Benjamin Perseghetti <[email protected]>
Allows for follow camera control set from sdf as well as gui.

Signed-off-by: Benjamin Perseghetti <[email protected]>

Co-authored-by: Jenn Nguyen <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
@bperseghetti bperseghetti force-pushed the pr-set-follow-camera-pgain branch from 935c53c to 568545f Compare January 7, 2024 18:46
bperseghetti and others added 2 commits January 7, 2024 13:56
White space cleanup.

Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
@jennuine
Copy link
Contributor

jennuine commented Feb 2, 2024

@bperseghetti there are CI errors:

/home/jenkins/workspace/gz_gui-ci-pr_any-focal-amd64/gz-gui/test/integration/follow_config.cc:162
Expected: (sleep) < (maxSleep), actual: 61 vs 60

@bperseghetti
Copy link
Member Author

@bperseghetti there are CI errors:

/home/jenkins/workspace/gz_gui-ci-pr_any-focal-amd64/gz-gui/test/integration/follow_config.cc:162
Expected: (sleep) < (maxSleep), actual: 61 vs 60

I actually was planning to close this and open a new PR that has a more "holistic" approach to controlling the follow camera including what was done in here but more.

I just need to rebase that work on humble as I no longer plan to target it to garden (due to relatively sooner EOL).

@bperseghetti
Copy link
Member Author

Closing in favor of PR #619

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.

No ability to make follow camera "fixed" or set pgain.
7 participants