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

Set View Camera controller from plugin configuration #506

Merged
merged 5 commits into from
Dec 1, 2022

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Nov 22, 2022

Signed-off-by: Alejandro Hernández Cordero [email protected]

🎉 New feature

Summary

Set View Camera controller from plugin configuration

      <!-- 3D scene -->
      <plugin filename="MinimalScene" name="3D View">
        <ignition-gui>
          <title>3D View</title>
          <property type="bool" key="showTitleBar">false</property>
          <property type="string" key="state">docked</property>
        </ignition-gui>

        <engine>ogre2</engine>
        <scene>scene</scene>
        <ambient_light>0.4 0.4 0.4</ambient_light>
        <background_color>0.8 0.8 0.8</background_color>
        <camera_pose>-6 0 6 0 0.5 0</camera_pose>
        <camera_clip>
          <near>0.25</near>
          <far>25000</far>
        </camera_clip>
        <view_controller>ortho</view_controller>
      </plugin>

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 (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: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde requested a review from jennuine as a code owner November 22, 2022 23:20
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Nov 22, 2022
@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #506 (296836f) into ign-gui6 (0e4a198) will increase coverage by 1.25%.
The diff coverage is 100.00%.

❗ Current head 296836f differs from pull request most recent head 4ef3676. Consider uploading reports for the commit 4ef3676 to get more accurate results

@@             Coverage Diff              @@
##           ign-gui6     #506      +/-   ##
============================================
+ Coverage     66.85%   68.10%   +1.25%     
============================================
  Files            39       39              
  Lines          5310     5327      +17     
============================================
+ Hits           3550     3628      +78     
+ Misses         1760     1699      -61     
Impacted Files Coverage Δ
src/plugins/minimal_scene/MinimalScene.cc 61.17% <100.00%> (+1.17%) ⬆️
...interactive_view_control/InteractiveViewControl.cc 49.48% <0.00%> (+31.44%) ⬆️

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

@azeey azeey requested a review from iche033 November 28, 2022 19:50
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.

looks good to me, just one minor suggestion

src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde requested a review from jennuine November 29, 2022 16:39
Comment on lines 349 to 351
if (!_result)
ignerr << "Error setting view controller. Check if view angle GUI "
"plugin is already loaded" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit (curly braces and remove already):

Suggested change
if (!_result)
ignerr << "Error setting view controller. Check if view angle GUI "
"plugin is already loaded" << std::endl;
if (!_result)
{
ignerr << "Error setting view controller. Check if the View Angle GUI "
<< "plugin is loaded." << std::endl;
}

"plugin is already loaded" << std::endl;
else
{
this->cameraViewController = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we always want to set this back to an empty string regardless if _result is true or false? If _result == false, then the request will be sent at each Render() call

src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.hh Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.hh Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.hh Outdated Show resolved Hide resolved
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde requested a review from jennuine November 30, 2022 17:34
@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 30, 2022

@jennuine Added feedback here 0f7e805

Signed-off-by: Jenn Nguyen <[email protected]>
@jennuine jennuine force-pushed the ahcorde/6/minimal_scene/camera_view_controller branch from 559c9e1 to 4ef3676 Compare November 30, 2022 23:54
Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I added a test to help with code coverage 4ef3676

Feel free to merge if you're ok with the changes

@ahcorde ahcorde merged commit b3468ab into ign-gui6 Dec 1, 2022
@ahcorde ahcorde deleted the ahcorde/6/minimal_scene/camera_view_controller branch December 1, 2022 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants