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

ompl_interface::ConstrainedGoalSampler::sampleUsingConstraintSampler doesn't appear to sample? #2811

Closed
werner291 opened this issue Aug 13, 2021 · 6 comments

Comments

@werner291
Copy link
Contributor

Hello,

I was working on trying to implement a custom constraint_samplers::ConstraintSampler, but I noticed that only the project method gets called, never the sample method.

The state I get as input always appears to be the same (I think it's the same as the starting state?) when I print it out.

When reading through https://github.com/ros-planning/moveit/blob/master/moveit_planners/ompl/ompl_interface/src/detail/constrained_goal_sampler.cpp#L90, I also don't really see any source of randomness in there... it appears to just be projecting the same state over and over.

Is this the intended behavior?

@werner291
Copy link
Contributor Author

Looks like sample can be swapped into the place where project is used with no further changes to the code; it even takes the same arguments. Maybe that was what was supposed to happen there?

@werner291
Copy link
Contributor Author

werner291 commented Aug 13, 2021

Made the swap in my own fork of the code... so far seems to work fine?

@v4hn
Copy link
Contributor

v4hn commented Aug 28, 2021

Maybe that was what was supposed to happen there?

project was apparently just treated as another interface for sampling here where the first parameter serves as input and output.
From what I can see, the MoveIt code base only uses project() at the moment and if you change it to sample, then project will be unused. So they are basically redundant interfaces in case some plugin and user code wants to differentiate. Clearly no nice design.

I'm open to fix the semantics here. If you want to change it to

constraint_sampler_->sample(work_state_, work_state_, planning_context_->getMaximumStateSamplingAttempts()))

please file a pull-request.

werner291 added a commit to werner291/moveit that referenced this issue Sep 10, 2021
This is a PR for moveit#2811
@werner291 werner291 mentioned this issue Sep 10, 2021
6 tasks
@werner291
Copy link
Contributor Author

Well, the semantics seem quite different, don't they?

The way the documentation is written, "sample" generates a completely new robot state, writing it into the passed-in data-structure, whereas "project" actually takes the passed-in structure, and applies an operation to it that makes it compliant with the constraints.

Either way, see the new pull request: #2872

@simonschmeisser
Copy link
Contributor

Please also have a look at #2273 It was finished and merged in MoveIt2 but unfortunately not in MoveIt (1) Maybe it could be backported? (If that is what you need in the end ...)

@werner291
Copy link
Contributor Author

Oooh, that looks fancy! I'll check it out; I've never really been a huge fan of the "carthesian planning + IK" approach, since IK is kinda meaningless for my particular robot (base is very mobile) without directly tying it to the planning process.

At the moment though, I'm just using a constrained sampler, which seems to work just fine for my purpose.

I still believe my PR (#2872) would be useful, since in my particular sampler, projecting and sampling really are two distinct actions; one relies on the other, actually. (It produces samples uniformly, then projects them to obtain constrained ones.)

I can put all of my code in the project, method, but that feels kinda wrong, and I'd prefer not to depend on what appears to me to be a bug in MoveIt.

@v4hn v4hn closed this as completed in a560783 Sep 14, 2021
v4hn added a commit to v4hn/moveit that referenced this issue Jul 19, 2022
This reverts commit a560783.

It introduced severe regressions when planning with PoseGoals
because it would pass in random seeds into IK instead of the current state.
simonschmeisser added a commit to isys-vision/moveit that referenced this issue Aug 4, 2022
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

No branches or pull requests

3 participants