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

Add named logging with LOGNAME to OMPL interface #2211

Merged
merged 5 commits into from
Jul 21, 2020

Conversation

JeroenDM
Copy link
Contributor

Description

As is done in other parts of MoveIt #1079 #1179, this pull request adds named logging to all source files of the moveit_planners_ompl package. (For some files with a single log statement, it might be overkill, but it is consistent...)

Important I'm not confident whether it is ok to put this outside a namespace. But in most source files of the OMPL interface, all code is put outside of a namespace and all names are prepended with ompl_interface::. Maybe this is worth another cleanup? Or is there a reason for this?

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@JeroenDM JeroenDM requested a review from mamoll as a code owner July 17, 2020 08:43
@JeroenDM
Copy link
Contributor Author

I think I used the wrong clang-format configurations, I will fix it.

@JeroenDM
Copy link
Contributor Author

Tavis reports failing tests in moveit/moveit_ros/perception/mesh_filter/test/mesh_filter_test. I don't know what to make of it.

@rhaschke
Copy link
Contributor

The failing mesh_filter tests are unrelated (see #2212 and a pending fix in #2180).

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

makes sense

@JeroenDM
Copy link
Contributor Author

Just curious, is there another way to quickly check which tests failed on Travis, besides reading through the XML output?

@v4hn
Copy link
Contributor

v4hn commented Jul 17, 2020

The short summaries are listed at the bottom of the log: https://travis-ci.com/github/ros-planning/moveit/jobs/362068846#L1837

@JeroenDM
Copy link
Contributor Author

Aaah, I looked over it, sorry. Thank you :)

Copy link
Contributor

@mamoll mamoll left a comment

Choose a reason for hiding this comment

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

I requested some small changes, but overall this looks good. Thanks!

@v4hn
Copy link
Contributor

v4hn commented Jul 17, 2020 via email

@JeroenDM
Copy link
Contributor Author

I must admit I don't really understand what I did.

From what I understand (after some googling) the constexpr adheres to normal namespace behavior in this context. The LOGNAME's in the rest of the file work because they are used inside class methods that are defined inside the same namespace, and therefore LOGNAME is available.

Ideally, I would like to extend the namespace scope to the end of the source files, as done in ompl_planner_manager.cpp. It would make the code less verbose. But that's probably worth a separate issue / pull request.

@codecov
Copy link

codecov bot commented Jul 17, 2020

Codecov Report

Merging #2211 into master will decrease coverage by 0.00%.
The diff coverage is 18.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2211      +/-   ##
==========================================
- Coverage   57.71%   57.71%   -0.01%     
==========================================
  Files         326      326              
  Lines       25663    25662       -1     
==========================================
- Hits        14811    14810       -1     
  Misses      10852    10852              
Impacted Files Coverage Δ
...ace/src/detail/constrained_valid_state_sampler.cpp 2.85% <0.00%> (ø)
.../ompl_interface/src/detail/constraints_library.cpp 2.06% <0.00%> (ø)
...pl_interface/src/detail/state_validity_checker.cpp 27.52% <0.00%> (ø)
...s/ompl/ompl_interface/src/ompl_planner_manager.cpp 54.68% <0.00%> (ø)
...e/src/parameterization/model_based_state_space.cpp 73.10% <0.00%> (ø)
...meterization/work_space/pose_model_state_space.cpp 82.31% <0.00%> (ø)
...mpl_interface/src/model_based_planning_context.cpp 59.68% <26.66%> (ø)
...pl/ompl_interface/src/planning_context_manager.cpp 70.94% <30.76%> (ø)
...lanners/ompl/ompl_interface/src/ompl_interface.cpp 71.42% <33.33%> (ø)
..._interface/src/detail/constrained_goal_sampler.cpp 73.80% <50.00%> (ø)
... and 6 more

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 d6666a9...ec11614. Read the comment docs.

Copy link
Contributor

@v4hn v4hn 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 now and @mamoll's nit was integrated. Merging.

@v4hn v4hn merged commit 89a6752 into moveit:master Jul 21, 2020
@JeroenDM JeroenDM deleted the ompl-interface-cleanup branch July 31, 2020 06:13
@rhaschke rhaschke mentioned this pull request Aug 13, 2020
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
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.

4 participants