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

Visualize Frustum #1095

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

BA-Utkarsh
Copy link

@BA-Utkarsh BA-Utkarsh commented Dec 26, 2024

🎉 New feature

Summary

This PR mainly adds the visualization of Frustum.
We could see it was present in gazebo classic and from gazebo garden onwards the plugin/feature is not available.

Test it

$ Build gazebo from source.
$ . install/setup.sh
$ gz sim examples/worlds/visualize_frustum.sdf

Test Ref images,

  1. Play the simulation.
    frustum-1

  2. Select the topic from scroll down.
    frustum-2

  3. Refresh it to get the "logical_camera/frustum" topic.
    frustum-3

  4. Subcriibed to "logical_camera/frustum".
    frustum-4

  5. Final output
    frustum-6

Checklist

  • 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
  • All tests passed
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Supporting PRs

include/gz/rendering/FrustumVisual.hh Outdated Show resolved Hide resolved
include/gz/rendering/FrustumVisual.hh Outdated Show resolved Hide resolved
include/gz/rendering/FrustumVisual.hh Outdated Show resolved Hide resolved
include/gz/rendering/FrustumVisual.hh Outdated Show resolved Hide resolved
include/gz/rendering/base/BaseFrustumVisual.hh Outdated Show resolved Hide resolved
ogre2/src/Ogre2FrustumVisual.cc Show resolved Hide resolved
ogre2/src/Ogre2FrustumVisual.cc Show resolved Hide resolved
ogre2/src/Ogre2FrustumVisual.cc Show resolved Hide resolved
ogre2/src/Ogre2FrustumVisual.cc Show resolved Hide resolved
src/FrustumVisual.cc Outdated Show resolved Hide resolved
Signed-off-by: Utkarsh <[email protected]>
Signed-off-by: Utkarsh <[email protected]>
@BA-Utkarsh
Copy link
Author

@ahcorde , thank you for review. I have addressed all the review comments.

@BA-Utkarsh BA-Utkarsh requested a review from ahcorde January 10, 2025 07:45
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.

please check linters:

  /usr/share/gz/gz-cmake4/codecheck/cpplint.py:55: DeprecationWarning: module 'sre_compile' is deprecated
    import sre_compile
  /github/workspace/include/gz/rendering/FrustumVisual.hh:131:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/include/gz/rendering/Scene.hh:1121:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/include/gz/rendering/base/BaseFrustumVisual.hh:81:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/include/gz/rendering/base/BaseFrustumVisual.hh:84:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/include/gz/rendering/base/BaseFrustumVisual.hh:84:  Should have a space between // and comment  [whitespace/comments] [4]
  /github/workspace/include/gz/rendering/base/BaseFrustumVisual.hh:87:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/include/gz/rendering/base/BaseFrustumVisual.hh:87:  Should have a space between // and comment  [whitespace/comments] [4]
  /github/workspace/include/gz/rendering/base/BaseFrustumVisual.hh:222:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/include/gz/rendering/base/BaseFrustumVisual.hh:238:  Tab found; better to use spaces  [whitespace/tab] [1]
  /github/workspace/include/gz/rendering/base/BaseScene.hh:850:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre/include/gz/rendering/ogre/OgreScene.hh:184:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre/src/OgreFrustumVisual.cc:32:  Should have a space between // and comment  [whitespace/comments] [4]
  /github/workspace/ogre/src/OgreFrustumVisual.cc:33:  Tab found; better to use spaces  [whitespace/tab] [1]
  /github/workspace/ogre/src/OgreFrustumVisual.cc:33:  Should have a space between // and comment  [whitespace/comments] [4]
  /github/workspace/ogre/src/OgreFrustumVisual.cc:42:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre/src/OgreFrustumVisual.cc:94:  Should have a space between // and comment  [whitespace/comments] [4]
  /github/workspace/ogre2/include/gz/rendering/ogre2/Ogre2Scene.hh:347:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:61:  Should have a space between // and comment  [whitespace/comments] [4]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:62:  Tab found; better to use spaces  [whitespace/tab] [1]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:62:  Should have a space between // and comment  [whitespace/comments] [4]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:71:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:151:  Should have a space between // and comment  [whitespace/comments] [4]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:154:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:175:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:176:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:177:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:193:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:194:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:195:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:268:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:269:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:272:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:275:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:278:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:281:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:283:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/ogre2/src/Ogre2FrustumVisual.cc:284:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  Total errors found: 37

@BA-Utkarsh BA-Utkarsh requested a review from ahcorde January 11, 2025 10:29
@iche033
Copy link
Contributor

iche033 commented Jan 13, 2025

Thanks for the contribution!

We follow semantic versioning and promise to maintain ABI compatibility within the same major version. Since this is targeting a release branch, we can't break ABI. The ABI checker did pick up that the changes broke ABI (compatibility needs to be 100%). The breaking change is mainly the new CreateFrustumVisualImpl virtual function (which changed the vtable for BaseScene class).

Ideally we should re-target this PR to the main branch. Do you need this is gz-rendering9? If I not, let's just re-target to main. If you do need this in a released branch (gz-rendering9 or earlier), I can help implement a workaround to make it ABI-compatible. To do this, we should also get this in main first, and I'll backport this change with ABI compatibility changes.

@BA-Utkarsh
Copy link
Author

BA-Utkarsh commented Jan 15, 2025

Thanks for the contribution!

We follow semantic versioning and promise to maintain ABI compatibility within the same major version. Since this is targeting a release branch, we can't break ABI. The ABI checker did pick up that the changes broke ABI (compatibility needs to be 100%). The breaking change is mainly the new CreateFrustumVisualImpl virtual function (which changed the vtable for BaseScene class).

Ideally we should re-target this PR to the main branch. Do you need this is gz-rendering9? If I not, let's just re-target to main. If you do need this in a released branch (gz-rendering9 or earlier), I can help implement a workaround to make it ABI-compatible. To do this, we should also get this in main first, and I'll backport this change with ABI compatibility changes.

Thank you @iche033 for your comment and review.
Yes, I do need it in gz-rendering9.
I shall change the target to main. Please help me with the ABI compatibility.

@BA-Utkarsh BA-Utkarsh changed the base branch from gz-rendering9 to main January 15, 2025 06:49
@BA-Utkarsh
Copy link
Author

@iche033 , I changed the target branch to main, please let me know the next action items or you will take care from here? :)

@iche033
Copy link
Contributor

iche033 commented Jan 15, 2025

I created a PR (BA-Utkarsh#2) to your repo with some changes to function and variable names for consistency with the Camera class. I also added a frustum_visual demo, and addressed comments from other reviewers. Please take a look and merge if changes look good to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

3 participants