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

[Gazebo 9] Added test to check collisions equal to zero #2788

Merged
merged 4 commits into from
Jul 23, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 13, 2020

I missed to add tests in this PR #2768.

I added a world with a box, sphere and cylinder with collisions equal to zero. The test will try to load the world, no segmentation fault should happens

Signed-off-by: ahcorde ahcorde@gmail.com

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde added the 9 Gazebo 9 label Jul 13, 2020
@ahcorde ahcorde requested a review from mabelzhang July 13, 2020 10:07
@ahcorde ahcorde self-assigned this Jul 13, 2020
Copy link
Collaborator

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

I tried the test, it passes, but it also passes when I reverted to the original code that produced the GUI bug before the change in https://github.com/osrf/gazebo/pull/2768/files .

I reverted Visual.cc lines 814-823 to the original lines 814-815. In GUI, when I do View > Collisions, I get

gzclient: /build/ogre-1.9-B6QkmW/ogre-1.9-1.9.0+dfsg1/OgreMain/src/OgreNode.cpp:630: virtual void Ogre::Node::setScale(const Ogre::Vector3&): Assertion `!inScale.isNaN() && "Invalid vector supplied as parameter"' failed.
Aborted (core dumped)

But the test is passing with this original version of code. So it is not exposing the bug.

gazebo/rendering/Visual_TEST.cc Outdated Show resolved Hide resolved
@@ -1588,6 +1588,33 @@ TEST_F(Visual_TEST, ConvertVisualType)
rendering::Visual::ConvertVisualType(msgs::Visual::PHYSICS));
}

TEST_F(Visual_TEST, collisionZero)
{
// This test checks that there isn't a segmentation fault when inserting zero collision geometries
Copy link
Collaborator

Choose a reason for hiding this comment

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

Code check is saying lines 1593 and 1594 should be < 80 chars long.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I tried the test, it passes, but it also passes when I reverted to the original code

Maybe the failure condition can be triggered by calling scene->ShowCollisions(true).

gazebo/rendering/Visual_TEST.cc Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde changed the title Added test to check collisions equal to zero [Gazebo 9] Added test to check collisions equal to zero Jul 21, 2020
@mabelzhang
Copy link
Collaborator

Verified that now the test does fail with the error above when Visual.cc is reverted.
Test passes with the fix.

tools/code_check.sh still reporting these though:

./gazebo/rendering/Visual_TEST.cc:1593:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
./gazebo/rendering/Visual_TEST.cc:1595:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
./gazebo/rendering/Visual_TEST.cc:1619:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 22, 2020

Thank you @mabelzhang ! I got confused with the 100 characters per line in ROS

@mabelzhang
Copy link
Collaborator

Yeah I do that too. I don't even remember if ign-gazebo is 80 or 100.
I'll wait for Jenkins to finish.

Copy link
Collaborator

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

CI looks fine. Feel free to merge.

@ahcorde ahcorde merged commit 7dc6453 into gazebo9 Jul 23, 2020
@ahcorde ahcorde deleted the ahcorde/add/crash_collision_visual_test_gazebo9 branch July 23, 2020 08:01
ahcorde added a commit that referenced this pull request Jul 23, 2020
* Added test to check collisions equal to zero

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Included feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>

* make linters happy

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Update Visual_TEST.cc
ahcorde added a commit to ahcorde/gazebo that referenced this pull request Jul 23, 2020
* Added test to check collisions equal to zero

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Included feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>

* make linters happy

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Update Visual_TEST.cc
ahcorde added a commit that referenced this pull request Jul 24, 2020
* Added test to check collisions equal to zero

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Included feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>

* make linters happy

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Update Visual_TEST.cc
ahcorde added a commit that referenced this pull request Jul 28, 2020
* Fixed crash when collision size is zero

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Using ignition::math::isnan

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Improved error message

Signed-off-by: ahcorde <ahcorde@gmail.com>

* fixed method to get the name of the visual

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Fixed else brackets

Signed-off-by: ahcorde <ahcorde@gmail.com>

* [Gazebo 9] Added test to check collisions equal to zero (#2788)

* Added test to check collisions equal to zero

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Included feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>

* make linters happy

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Update Visual_TEST.cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9 Gazebo 9
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants