search

personalrobotics / prpy

Python utilities used by the Personal Robotics Laboratory.
BSD 3-Clause "New" or "Revised" License
62 stars 19 forks source link

Print warning if (attempting) to double clone an environment / Refactor workspace.py #256

Closed DavidB-CMU closed 8 years ago

DavidB-CMU commented 8 years ago

This is related to pull request https://github.com/personalrobotics/prpy/pull/55 ("Fixed a number of bugs related to workspace planner")

If you call a planning method like PlanToEndEffectorOffset() it then calls PlanWorkspacePath(). This results in two calls to Clone(), but there's a check in there

if self.clone_env != self.clone_parent:
    with self.clone_parent:
        self.clone_env.Clone(self.clone_parent, self.options)

which means that the environment is not cloned twice.

In https://github.com/personalrobotics/prpy/pull/55 @mkoval suggested that we should print a warning message like "Warning: you are trying to clone an environment into itself" Should we add this warning message?

I would like to suggest that we also re-factor workspace.py so Clone() is not called twice.

psigen commented 8 years ago

After doing some work on this in #272 and #279, I think the best option here is to simply make self-cloning be a near-free operation.

There are a lot of cases, like nested @PlanningMethod calls, where deduplicating cloning is a very verbose operation, either requiring refactoring tons of common code into private helper methods, or branching off a special case when the clone is not required.

Since with Clone() also performs locking and adds a .Cloned() operator to normalize references which may or may not be in the environment, it already handles a lot of edge cases for us. I think cloning into the same environment should be one of those cases, so we should make it so that there is no penalty for doing so.

Semantically, we will just be accessing/locking the one environment we would be using anyway, so I think this is a safe thing to do.

mkoval commented 8 years ago

:+1: I agree with @psigen analysis. Eliminating all nested @PlanningMethod introduces too much boilerplate to be a viable solution.

I suspect that #279 eliminates most overhead. I am going to close this issue, but please do not hesitate to re-open it if you notice performance issues from multiple Clone calls into the same environment.