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

Use std::optional instead of nullptr checking #2454

Merged
merged 13 commits into from
May 1, 2024

Conversation

Shobuj-Paul
Copy link
Contributor

@Shobuj-Paul Shobuj-Paul commented Oct 14, 2023

Description

Wrapped scene_transforms_, robot_state_, acm_ and object_types_ in std::optional to handle missing data instead of checking for nullptr.
Fixes #2131

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

@Shobuj-Paul Shobuj-Paul marked this pull request as draft October 14, 2023 18:57
@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

Attention: Patch coverage is 72.22222% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 42.98%. Comparing base (d962501) to head (39e6839).
Report is 19 commits behind head on main.

Files Patch % Lines
moveit_core/planning_scene/src/planning_scene.cpp 70.59% 25 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2454      +/-   ##
==========================================
- Coverage   50.74%   42.98%   -7.76%     
==========================================
  Files         392      692     +300     
  Lines       32553    56332   +23779     
  Branches        0     7274    +7274     
==========================================
+ Hits        16517    24208    +7691     
- Misses      16036    31961   +15925     
- Partials        0      163     +163     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! Something is still not working since the tests are complaining. Can you fix that?

moveit_core/planning_scene/src/planning_scene.cpp Outdated Show resolved Hide resolved
moveit_core/planning_scene/src/planning_scene.cpp Outdated Show resolved Hide resolved
@Shobuj-Paul Shobuj-Paul force-pushed the optional branch 2 times, most recently from 5a31411 to 2274cab Compare October 20, 2023 18:10
@Shobuj-Paul
Copy link
Contributor Author

Shobuj-Paul commented Oct 21, 2023

Thanks for tackling this! Something is still not working since the tests are complaining. Can you fix that?

The build is completing but the tests fail, working on sorting it out.

Putting this down for my own reference:
Okay so seems like a value can't be directly assigned to the reference returned by the value() function in optional (hence the "bad optional access" exceptions), so emplace() needs to be used instead. value_or() function only returns the value if exists or some other object, doesn't actually assign anything to the object itself.

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Most of these pointers are only pointers because they are optional. If we use std::optional, we should switch members to values as long as the API is only exposing references.

@Shobuj-Paul
Copy link
Contributor Author

Shobuj-Paul commented Oct 24, 2023

Most of these pointers are only pointers because they are optional. If we use std::optional, we should switch members to values as long as the API is only exposing references.

I have changed object_types_, robot_state_ and acm_ to values.
scene_transforms_ is giving some issues, here were are trying to assign it a SceneTransforms object, but here the object has been defined as the parent class Transforms object. The pointer implementation seems to be casting it, need to see how to do the same with values.

Can't seem to be making scene_transforms_ into a value successfully, open to suggestions.

Copy link

mergify bot commented Dec 17, 2023

This pull request is in conflict. Could you fix it @Shobuj-Paul?

@Shobuj-Paul
Copy link
Contributor Author

@sjahr
Could you review this PR?

Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

I like this change 👍 Thanks and sorry for the late response

@EzraBrooks EzraBrooks added this pull request to the merge queue May 1, 2024
Merged via the queue into moveit:main with commit 358ec63 May 1, 2024
11 of 12 checks passed
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.

Replace the nullpointer checking in the Planning Scene with std::optional
4 participants