Implement XDisplacement function for checking x displacement for all nodes #411

onurtore · 2022-08-04T09:18:33Z

🦟 Bug fix

Related to #393

Summary

During actor's trajectory creation, createActor function only looks for the root node's x displacement while setting the trajectory interpolation flag.
If root node does not have any animation, this results with a segmentation fault. While with the previous mesh loaders, this seg fault never happened, recent introduction of assimp loader create cases where root node does not have animation and results with segmentation fault during actor creation.

This patch introduces new function called XDisplacement to check all nodes inside the skelAnim for possible X displacement. This function will be utilized during the actor's trajectory creation.

Checklist

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

Implements XDisplacement function, function which looks all nodes for possible XDisplacement. Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

Cleans unnessary changes. Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

codecov · 2022-08-04T09:26:25Z

Codecov Report

Merging #411 (9bf235c) into gz-common5 (5d576ba) will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@              Coverage Diff               @@
##           gz-common5     #411      +/-   ##
==============================================
+ Coverage       80.38%   80.54%   +0.16%     
==============================================
  Files              85       85              
  Lines            9944     9956      +12     
==============================================
+ Hits             7993     8019      +26     
+ Misses           1951     1937      -14

Impacted Files	Coverage Δ
graphics/src/SkeletonAnimation.cc	`63.09% <100.00%> (+7.53%)`	⬆️
graphics/src/NodeAnimation.cc	`72.04% <0.00%> (+13.97%)`	⬆️

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

ahcorde

add some test

Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

ahcorde · 2022-08-18T13:22:48Z

graphics/src/AssimpLoader_TEST.cc

+  common::NodeAnimation *rootNode = skelAnim->NodeAnimationByName(rootNodeName);
+  EXPECT_EQ(nullptr, rootNode);
+  auto xDisplacement = skelAnim->XDisplacement();
+  ASSERT_TRUE(xDisplacement);


can we add another example below with that return xDisplacement=false?

Doing it, but might take a while, I need to understand how to remove animations from collada file format.

@ahcorde,
Can I create just an empty animation and test the function with that?

Done, added new test file for skeletonAnimation because we are not using any mesh for the new test.

@ahcorde ,
One question, I added new test file and re-compile everything using colcon, when I tried to make test, make didnt see any new test, I had to remove gz-common build and re-compile everything. Took me 30min, is there a easier way? Refreshing cache for test?

Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

ahcorde · 2022-08-18T14:40:49Z

graphics/src/SkeletonAnimation_TEST.cc

+  common::SkeletonAnimation* skelAnim =
+    new common::SkeletonAnimation("emptyAnimation");
+  auto xDisplacement = skelAnim->XDisplacement();
+  ASSERT_TRUE(!xDisplacement);


Suggested change

ASSERT_TRUE(!xDisplacement);

ASSERT_FALSE(xDisplacement);

Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

ahcorde · 2022-08-19T07:20:12Z

@osrf-jenkins run tests please!

ahcorde · 2022-08-19T07:45:14Z

Closed and reopened again, Github actions were stuck

ahcorde · 2022-08-19T08:38:14Z

@j-rivero @mjcarroll something is broken in the ABI checker

j-rivero · 2022-08-19T16:58:35Z

@j-rivero @mjcarroll something is broken in the ABI checker

Log of the ABI checker is:

...
ERROR: some errors occurred when compiling headers
ERROR: see log for details:
  /home/jenkins/workspace/ignition_common-abichecker-any_to_any-ubuntu_auto-amd64/abi_checker/logs/ign-common/X/log.txt

ERROR: can't compile header(s)

Note that the file described there is available via Jenkins workspace at https://build.osrfoundation.org/job/ignition_common-abichecker-any_to_any-ubuntu_auto-amd64/ws/abi_checker/logs/ign-common/X/log.txt/*view*/

The GCC parameters:
  gcc -fdump-lang-raw -fkeep-inline-functions -c -x c++ -fpermissive -w  -std=c++17 "/tmp/lCfTAc7aXw/dump1.h"  -I/usr/local/destination_branch/include/gz/common5 -I/usr/include/gz/utils2 -I/usr/include/gz/math7

In file included from /usr/local/destination_branch/include/gz/common5/gz/common/testing.hh:23,
                 from /tmp/lCfTAc7aXw/dump1.h:83:
/usr/local/destination_branch/include/gz/common5/gz/common/testing/AutoLogFixture.hh:20:10: fatal error: gtest/gtest.h: No such file or directory
   20 | #include <gtest/gtest.h>
      |          ^~~~~~~~~~~~~~~
compilation terminated.

Something seems to have broken the compilation in the gz-common5 branch. Would you mind creating an issue for it please?

luca-della-vedova · 2022-08-23T10:42:48Z

Did you try to use this function on an actual animated model that has no X displacement (i.e. this)?
My concern is that this change will return much more positives. For example if there is even one node that has a pose which has a X in its last keyframe that is slightly different from the first keyframe (i.e. the hand bone for a person looking at their phone while being on the spot) this function might return true while previously we would have considered that false since the root was not moving.

onurtore · 2022-08-23T10:50:32Z

@luca-della-vedova,
What is the alternative that will works with assimp? Accesing child node? And what if the root node does have two child node (still I did not see any case that is possible) ? Then which one we should use?

onurtore · 2022-08-23T10:55:13Z

@luca-della-vedova,
Is there a specific reason why we only check for root node and if there is, what is the reason.

onurtore · 2022-08-23T17:03:50Z

Hi @iche033,
I think you wrote the code for checking x interpolation for animations during actor creations. The current code checks only for root node, is there a specific reason for that?

This PR makes it check every node for x interpolation. But I dont know, whether this is something correct.

Onur Berk Tore added 4 commits August 4, 2022 11:54

XDisplacement function

a27f525

Implements XDisplacement function, function which looks all nodes for possible XDisplacement. Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

Cleanup

b3bc264

Cleans unnessary changes. Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

Added documentation

6aac712

Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

Fix documentation

acb48d5

Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

onurtore requested a review from mjcarroll as a code owner August 4, 2022 09:18

github-actions bot added the 🌱 garden Ignition Garden label Aug 4, 2022

onurtore changed the title ~~Otore19/fixin seg fault~~ Defines XDisplacement function for checking X displacement for all nodes Aug 4, 2022

onurtore mentioned this pull request Aug 4, 2022

[Meta-ticket][GSoC22] Improving Gazebo’s Mesh Format Support #374

Open

8 tasks

onurtore changed the title ~~Defines XDisplacement function for checking X displacement for all nodes~~ Implement XDisplacement function for checking x displacement for all nodes Aug 4, 2022

chapulina added the bug Something isn't working label Aug 5, 2022

chapulina requested a review from luca-della-vedova August 5, 2022 01:37

chapulina changed the base branch from main to gz-common5 August 5, 2022 19:05

onurtore mentioned this pull request Aug 7, 2022

Use assimp for loading meshes #393

Merged

9 tasks

ahcorde requested changes Aug 16, 2022

View reviewed changes

onurtore mentioned this pull request Aug 17, 2022

Uses new XDisplacement func to check X displacement gazebosim/gz-sim#1655

Closed

8 tasks

ahcorde assigned onurtore Aug 18, 2022

Adds a test for Xdisplacement func

dfa088f

Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

onurtore requested review from ahcorde and removed request for mjcarroll and luca-della-vedova August 18, 2022 13:14

ahcorde requested changes Aug 18, 2022

View reviewed changes

Introduces new test

8f3da8c

Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

onurtore requested a review from ahcorde August 18, 2022 14:37

Merge branch 'gz-common5' into otore19/fixin_seg_fault

e2deec6

ahcorde requested changes Aug 18, 2022

View reviewed changes

Updates assertion

9bf235c

Signed-off-by: Onur Berk Tore <otore19@ku.edu.tr>

onurtore requested a review from ahcorde August 18, 2022 14:49

ahcorde approved these changes Aug 19, 2022

View reviewed changes

ahcorde closed this Aug 19, 2022

ahcorde reopened this Aug 19, 2022

mjcarroll mentioned this pull request Aug 19, 2022

Fix ABI checker with testing module #427

Merged

8 tasks

chapulina merged commit 68b5a7c into gazebosim:gz-common5 Aug 30, 2022

onurtore mentioned this pull request Nov 16, 2022

Introduces remove scene node env var, renames func #424

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement XDisplacement function for checking x displacement for all nodes #411

Implement XDisplacement function for checking x displacement for all nodes #411

onurtore commented Aug 4, 2022 •

edited

Loading

codecov bot commented Aug 4, 2022 •

edited

Loading

ahcorde left a comment

ahcorde Aug 18, 2022

onurtore Aug 18, 2022 •

edited

Loading

onurtore Aug 18, 2022

ahcorde Aug 18, 2022

onurtore Aug 18, 2022

onurtore Aug 18, 2022 •

edited

Loading

ahcorde Aug 18, 2022

onurtore Aug 18, 2022

ahcorde commented Aug 19, 2022

ahcorde commented Aug 19, 2022

ahcorde commented Aug 19, 2022

j-rivero commented Aug 19, 2022

luca-della-vedova commented Aug 23, 2022

onurtore commented Aug 23, 2022 •

edited

Loading

onurtore commented Aug 23, 2022 •

edited

Loading

onurtore commented Aug 23, 2022 •

edited

Loading

Implement XDisplacement function for checking x displacement for all nodes #411

Implement XDisplacement function for checking x displacement for all nodes #411

Conversation

onurtore commented Aug 4, 2022 • edited Loading

🦟 Bug fix

Summary

Checklist

codecov bot commented Aug 4, 2022 • edited Loading

Codecov Report

ahcorde left a comment

Choose a reason for hiding this comment

ahcorde Aug 18, 2022

Choose a reason for hiding this comment

onurtore Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

onurtore Aug 18, 2022

Choose a reason for hiding this comment

ahcorde Aug 18, 2022

Choose a reason for hiding this comment

onurtore Aug 18, 2022

Choose a reason for hiding this comment

onurtore Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

ahcorde Aug 18, 2022

Choose a reason for hiding this comment

onurtore Aug 18, 2022

Choose a reason for hiding this comment

ahcorde commented Aug 19, 2022

ahcorde commented Aug 19, 2022

ahcorde commented Aug 19, 2022

j-rivero commented Aug 19, 2022

luca-della-vedova commented Aug 23, 2022

onurtore commented Aug 23, 2022 • edited Loading

onurtore commented Aug 23, 2022 • edited Loading

onurtore commented Aug 23, 2022 • edited Loading

onurtore commented Aug 4, 2022 •

edited

Loading

codecov bot commented Aug 4, 2022 •

edited

Loading

onurtore Aug 18, 2022 •

edited

Loading

onurtore Aug 18, 2022 •

edited

Loading

onurtore commented Aug 23, 2022 •

edited

Loading

onurtore commented Aug 23, 2022 •

edited

Loading

onurtore commented Aug 23, 2022 •

edited

Loading