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

Scale BaseAxis properly #88

Merged
merged 12 commits into from
May 29, 2020
Merged

Scale BaseAxis properly #88

merged 12 commits into from
May 29, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented May 22, 2020

While I was adding the origin axes to ignition gazebo I checked that the scale for a BaseAxis is wrong. Current implementation:

scale_wrong

Overriding the scale methods:

scale_ok

Signed-off-by: ahcorde [email protected]

iche033 and others added 4 commits May 19, 2020 19:53
@ahcorde ahcorde requested a review from iche033 as a code owner May 22, 2020 10:38
@github-actions github-actions bot added the 🔮 dome Ignition Dome label May 22, 2020
@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #88 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #88   +/-   ##
======================================
  Coverage    7.56%   7.56%           
======================================
  Files          26      26           
  Lines        1744    1744           
======================================
  Hits          132     132           
  Misses       1612    1612           

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 bcd9c49...ca745c9. Read the comment docs.


public: virtual void SetLocalScale(double _scale) override;

public: virtual void SetLocalScale(const math::Vector3d &_scale) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should be able to just override this version with math::Vector3d arg. The other two just calls this function so can be removed.

template <class T>
void BaseAxisVisual<T>::SetLocalScale(const math::Vector3d &_scale)
{
xArrow->SetLocalScale(_scale.X(), _scale.Y(), _scale.Z());
Copy link
Contributor

Choose a reason for hiding this comment

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

how about:

      for (unsigned int i = 0; i < this->ChildCount(); ++i)
        this->ChildByIndex(i)->SetLocalScale(_scale);

this way we don't need to declare the private xArrow, yArrow, zArrow variables

@ahcorde
Copy link
Contributor Author

ahcorde commented May 25, 2020

This PR builds on top of this other PR #87

Signed-off-by: Ian Chen <[email protected]>
@iche033
Copy link
Contributor

iche033 commented May 26, 2020

looks good, updated change log in 71f4e44

@ahcorde
Copy link
Contributor Author

ahcorde commented May 27, 2020

there is an issue in one of the test https://build.osrfoundation.org/job/ignition_rendering-ci-pr_any-ubuntu_auto-amd64/623/testReport/junit/(root)/AxisVisual_AxisVisualTest/AxisVisual_ogre/

/var/lib/jenkins/workspace/ignition_rendering-ci-pr_any-ubuntu_auto-amd64/ign-rendering/src/AxisVisual_TEST.cc:65
Expected equality of these values:
 math::Vector3d(0.2, 0.3, 0.4)
   Which is: 0.2 0.3 0.4
 visual->LocalScale()
   Which is: 1 1 1

@ahcorde
Copy link
Contributor Author

ahcorde commented May 28, 2020

CI is green except on windows

@iche033
Copy link
Contributor

iche033 commented May 28, 2020

tests on windows are not fully working yet so the failures are expected

@ahcorde ahcorde merged commit 5025487 into master May 29, 2020
@ahcorde ahcorde deleted the ahcorde/fix/scale_baseaxis branch May 29, 2020 06:36
@chapulina
Copy link
Contributor

tests on windows are not fully working yet so the failures are expected

Just an FYI that the buildfarmer's script indicates that these 2 failures are new on Windows:

    AxisVisual/AxisVisualTest.AxisVisual/ogre
    ArrowVisual/ArrowVisualTest.ArrowVisual/ogre

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants