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

Moved ScreenToPlane and ScreenToScene from ign-gui to ign-rendering #363

Merged
merged 8 commits into from
Jul 20, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 16, 2021

🎉 New feature

Summary

Moved ScreenToPlane and ScreenToScene from ign-gui to ign-rendering. Some of the small plugins that I'm creating to consolidate the Scene3D use these methods.

@scpeters do you need to retarget this PR to another branch ?

Checklist

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

@ahcorde ahcorde requested a review from scpeters July 16, 2021 11:02
@ahcorde ahcorde self-assigned this Jul 16, 2021
@ahcorde ahcorde requested a review from iche033 as a code owner July 16, 2021 11:02
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jul 16, 2021
@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #363 (7d864e1) into ign-rendering3 (5164539) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           ign-rendering3     #363      +/-   ##
==================================================
+ Coverage           53.23%   53.35%   +0.12%     
==================================================
  Files                 131      131              
  Lines               12007    12036      +29     
==================================================
+ Hits                 6392     6422      +30     
+ Misses               5615     5614       -1     
Impacted Files Coverage Δ
include/ignition/rendering/RayQuery.hh 100.00% <100.00%> (ø)
src/Utils.cc 75.00% <100.00%> (+39.70%) ⬆️
ogre2/src/Ogre2RayQuery.cc 90.90% <0.00%> (+1.51%) ⬆️

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 5164539...7d864e1. Read the comment docs.

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 reason I wanted to change the ClickToScene events is to give more data, but ScreenToScene only returns a math::Vector3d. Getting a copy of the RayQueryResult from line 52 would be most useful to downstream applications, so you can know if you clicked on something or not and even the ign-rendering id of what was clicked on.

I think you could have a 2nd API:

math::Vector3d ScreenToScene(
    const math::Vector2i &,
    const CameraPtr &_camera,
    const RayQueryPtr &,
    RayQueryResult &);

so you could pass a reference to a RayQueryResult if you want the extra information

I think if we back port this to ign-gui3, it could benefit to citadel users

cc @francocipollone

src/Utils.cc Outdated Show resolved Hide resolved
@ahcorde ahcorde force-pushed the ahcorde/feature/clickto branch from 5e8454d to efa50b8 Compare July 19, 2021 15:15
@ahcorde ahcorde changed the base branch from main to ign-rendering3 July 19, 2021 15:15
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 19, 2021

Retargeted to ign-rendering3 (Citadel)

@scpeters scpeters added 🏰 citadel Ignition Citadel and removed 🏯 fortress Ignition Fortress labels Jul 19, 2021
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from scpeters July 20, 2021 12:13
Signed-off-by: Steven Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member

I just made some minor Doxygen fixes in 4f85db9

@scpeters
Copy link
Member

I've made some improvements to the test in branch scpeters/feature/clickto:

ahcorde/feature/clickto...scpeters/feature/clickto

Please consider merging or cherry-picking them into this branch if you approve. The change to RayQuery.hh was needed for the test to compile on my macOS machine

scpeters and others added 4 commits July 20, 2021 16:54
Signed-off-by: Steven Peters <scpeters@openrobotics.org>
* Use tighter tolerances in expectations
* Use IGN_PI/2 instead of 1.57 to tighten X tolerance
* Clarify comments about max distance, which is only used
  when the ray does not intersect an object
* Add expectations on RayQueryResult values
* Add test of API without RayQueryResult

Signed-off-by: Steven Peters <scpeters@openrobotics.org>
Signed-off-by: Steven Peters <scpeters@openrobotics.org>
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.

thanks, this is looking good. I just thought of one more thing to test and added it in 7d864e1

@ahcorde ahcorde merged commit dbbc3d7 into ign-rendering3 Jul 20, 2021
@ahcorde ahcorde deleted the ahcorde/feature/clickto branch July 20, 2021 20:57
/// \param[in] _maxDistance maximum distance to check the collision
/// \return 3D coordinates of a point in the 3D scene.
IGNITION_RENDERING_VISIBLE
math::Vector3d ScreenToScene(
Copy link
Contributor

Choose a reason for hiding this comment

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

global functions start with lower case. Can we change this to screenToScene? Same goes for the other functions added in this class.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh I'm a few mins late

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahcorde ahcorde mentioned this pull request Jul 20, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants