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

Remove unnecessary subframes map copy #2850

Merged
merged 2 commits into from
Aug 28, 2021

Conversation

felixvd
Copy link
Contributor

@felixvd felixvd commented Aug 27, 2021

As noted in #2816, I left a quick-and-dirty solution in #2037 . This commit was supposed to fix that, but it has a bug.

It's probably simple, so I'm putting this up for someone to make me look stupid.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I didn't look in detail. But is it ensured that both maps have the same entries before iterating over them in sync?
Alternatively, did you consider using std::transform?

What kind of bug do you observe? Crash? Wrong behavior?

@rhaschke
Copy link
Contributor

I prefer the commited version. Using std::transform requires to reset the global map on each update (which is less efficient):

  {
    auto& local = obj->subframe_poses_;
    auto& global = obj->global_subframe_poses_;
    global.clear();
    std::transform(local.begin(), local.end(), std::inserter(global, global.end()),
                   [&obj](moveit::core::FixedTransformsMap::value_type& input) {
                     return std::make_pair(input.first, obj->pose_ * input.second);
                   });
  }

@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #2850 (16aef15) into master (b170d62) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2850      +/-   ##
==========================================
+ Coverage   60.87%   60.88%   +0.02%     
==========================================
  Files         366      366              
  Lines       31705    31711       +6     
==========================================
+ Hits        19296    19305       +9     
+ Misses      12409    12406       -3     
Impacted Files Coverage Δ
moveit_core/collision_detection/src/world.cpp 89.66% <100.00%> (+0.13%) ⬆️
moveit_core/robot_state/src/robot_state.cpp 51.65% <0.00%> (-<0.01%) ⬇️
...bot_state/include/moveit/robot_state/robot_state.h 85.04% <0.00%> (ø)
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 78.25% <0.00%> (+0.39%) ⬆️
...on/pick_place/src/approach_and_translate_stage.cpp 74.03% <0.00%> (+0.65%) ⬆️
...meterization/work_space/pose_model_state_space.cpp 83.02% <0.00%> (+1.89%) ⬆️

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 b170d62...16aef15. Read the comment docs.

@felixvd
Copy link
Contributor Author

felixvd commented Aug 28, 2021

Thanks! I knew it was something simple :)

So the map copy still happens after all, but at least it is avoided when only the object pose is updated. Happy with this as is, feel free to merge.

@felixvd felixvd changed the title Fixme: Fix inefficient subframes map copy Remove unnecessary subframes map copy Aug 28, 2021
@rhaschke rhaschke merged commit 3c22249 into moveit:master Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants