https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
Summary: Updated getChain function
Product: KDL
Version: kdl-trunk
Platform: All
OS/Version: All
Status: NEW
Severity: normal
Priority: P3
Component: Kinematic chains
AssignedTo: orocos-dev [..] ...
ReportedBy: meeussen [..] ...
CC: orocos-dev [..] ...
Estimated Hours: 0.0
I wanted to propose a patch for the getChain function on a kdl Tree. The patch
changes 2 things:
1) the getChain function returns true/false to indicate success or failure.
This boils down to a check if a chain between the root name and tip name
exists.
2) the getChain function returns a vector of all the segment names in the newly
constructed chain. In a kdl Tree all segments have a name, but in a Chain this
is not the case. So, when you extract a chain from a tree, this information is
lost. Therefore it is useful to know which segments are actually in the chain
that is returned.
Wim
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
Ruben Smits <ruben [dot] smits [..] ...> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution| |FIXED
Status|NEW |RESOLVED
--- Comment #12 from Ruben Smits <ruben [dot] smits [..] ...> 2009-08-03 10:36:52 ---
If an issue about backwards compatibility pops up. Please open a new bug for
this.
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
--- Comment #11 from Wim Meeussen <meeussen [..] ...> 2009-07-24 00:06:16 ---
Created an attachment (id=481)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=481)
Patch with backwards compatible joint/segment constructors
[Bug 682] Updated getChain function
I looked at the patch but I do not like the changes to the addChain
and addTree functions, couldn't you just overload the functions? So we
can still use the prefix versions and the new versions? This way one
chain description can be added multiple times to the same tree, for
example two of the same arms on a torso.
Ruben
On Fri, Jul 24, 2009 at 12:06 AM, Wim Meeussen<meeussen [..] ...> wrote:
> https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
>
>
>
>
>
> --- Comment #11 from Wim Meeussen <meeussen [..] ...> 2009-07-24 00:06:16 ---
> Created an attachment (id=481)
> --> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=481)
> Patch with backwards compatible joint/segment constructors
>
> --
> Configure bugmail: https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You are on the CC list for the bug.
> You are the assignee for the bug.
> --
> Orocos-Dev mailing list
> Orocos-Dev [..] ...
> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev
>
[Bug 682] Updated getChain function
> I looked at the patch but I do not like the changes to the addChain
> and addTree functions, couldn't you just overload the functions? So we
> can still use the prefix versions and the new versions? This way one
> chain description can be added multiple times to the same tree, for
> example two of the same arms on a torso.
Yes, we can do that by changing the names of a segment when we add it
to a chain/tree.
Wim
>> --- Comment #11 from Wim Meeussen <meeussen [..] ...> 2009-07-24 00:06:16 ---
>> Created an attachment (id=481)
>> --> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=481)
>> Patch with backwards compatible joint/segment constructors
>>
>> --
>> Configure bugmail: https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
>> ------- You are receiving this mail because: -------
>> You are on the CC list for the bug.
>> You are the assignee for the bug.
>> --
>> Orocos-Dev mailing list
>> Orocos-Dev [..] ...
>> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev
>>
>
[Bug 682] Updated getChain function
On Thu, Jul 30, 2009 at 6:40 PM, Wim Meeussen<meeussen [..] ...> wrote:
>> I looked at the patch but I do not like the changes to the addChain
>> and addTree functions, couldn't you just overload the functions? So we
>> can still use the prefix versions and the new versions? This way one
>> chain description can be added multiple times to the same tree, for
>> example two of the same arms on a torso.
>
> Yes, we can do that by changing the names of a segment when we add it
> to a chain/tree.
But what about adding an already compiled model of an arm, that you
want to reuse while creating a tree? I already have some use-cases for
this, where we re-use the kinematic model of the Kuka-LBR multiple
times in the same tree.
Ruben
> Wim
>
>
>
>
>>> --- Comment #11 from Wim Meeussen <meeussen [..] ...> 2009-07-24 00:06:16 ---
>>> Created an attachment (id=481)
>>> --> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=481)
>>> Patch with backwards compatible joint/segment constructors
>>>
>>> --
>>> Configure bugmail: https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
>>> ------- You are receiving this mail because: -------
>>> You are on the CC list for the bug.
>>> You are the assignee for the bug.
>>> --
>>> Orocos-Dev mailing list
>>> Orocos-Dev [..] ...
>>> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev
>>>
>>
>
>
>
> --
> --
> Wim Meeussen
> Willow Garage Inc.
> <http://www.willowgarage.com)
>
[Bug 682] Updated getChain function
>> Yes, we can do that by changing the names of a segment when we add it
>> to a chain/tree.
>
> But what about adding an already compiled model of an arm, that you
> want to reuse while creating a tree? I already have some use-cases for
> this, where we re-use the kinematic model of the Kuka-LBR multiple
> times in the same tree.
We can do exactly the same for adding chains and trees to other chains/trees.
>>>> --- Comment #11 from Wim Meeussen <meeussen [..] ...> 2009-07-24 00:06:16 ---
>>>> Created an attachment (id=481)
>>>> --> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=481)
>>>> Patch with backwards compatible joint/segment constructors
>>>>
>>>> --
>>>> Configure bugmail: https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
>>>> ------- You are receiving this mail because: -------
>>>> You are on the CC list for the bug.
>>>> You are the assignee for the bug.
>>>> --
>>>> Orocos-Dev mailing list
>>>> Orocos-Dev [..] ...
>>>> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev
>>>>
>>>
>>
>>
>>
>> --
>> --
>> Wim Meeussen
>> Willow Garage Inc.
>> <http://www.willowgarage.com)
>>
>
[Bug 682] Updated getChain function
I committed the patch without adding the prefixes:
svn ci -m"added names to joints and segments and added a getChain
function to the Tree"
Sending src/joint.cpp
Sending src/joint.hpp
Sending src/segment.cpp
Sending src/segment.hpp
Sending src/tree.cpp
Sending src/tree.hpp
Sending tests/kinfamtest.cpp
Sending tests/solvertest.cpp
Transmitting file data ........
Committed revision 30400.
But i removed the throwing of an exception in the Tree::getSegment
function, we do not throw exceptions in functions that can be called
in realtime. It's up to the one that uses the result of getSegment to
check if the iterator is valid.
Since the models still compile, OCL still compiles and the tests still
compile, i consider this patch BC ;)
Ruben
On Thu, Jul 30, 2009 at 9:26 PM, Wim Meeussen<meeussen [..] ...> wrote:
>>> Yes, we can do that by changing the names of a segment when we add it
>>> to a chain/tree.
>>
>> But what about adding an already compiled model of an arm, that you
>> want to reuse while creating a tree? I already have some use-cases for
>> this, where we re-use the kinematic model of the Kuka-LBR multiple
>> times in the same tree.
>
> We can do exactly the same for adding chains and trees to other chains/trees.
>
>
>
>
>>>>> --- Comment #11 from Wim Meeussen <meeussen [..] ...> 2009-07-24 00:06:16 ---
>>>>> Created an attachment (id=481)
>>>>> --> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=481)
>>>>> Patch with backwards compatible joint/segment constructors
>>>>>
>>>>> --
>>>>> Configure bugmail: https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
>>>>> ------- You are receiving this mail because: -------
>>>>> You are on the CC list for the bug.
>>>>> You are the assignee for the bug.
>>>>> --
>>>>> Orocos-Dev mailing list
>>>>> Orocos-Dev [..] ...
>>>>> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev
>>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> --
>>> Wim Meeussen
>>> Willow Garage Inc.
>>> <http://www.willowgarage.com)
>>>
>>
>
>
>
> --
> --
> Wim Meeussen
> Willow Garage Inc.
> <http://www.willowgarage.com)
>
[Bug 682] Updated getChain function
> But i removed the throwing of an exception in the Tree::getSegment
> function, we do not throw exceptions in functions that can be called
> in realtime. It's up to the one that uses the result of getSegment to
> check if the iterator is valid.
I don't think there is a way to check if an iterator is valid if you
cannot compare it to map.end(), and we don't give the user access to
our segments map. We could change the function to: bool
getSegment(const std::string& segment_name,
SegmentMap::const_iterator& segment)
Wim
--
Wim Meeussen
Willow Garage Inc.
<http://www.willowgarage.com)
>
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
--- Comment #10 from Wim Meeussen <meeussen [..] ...> 2009-07-23 00:27:03 ---
> Guess who's reading your email :-)
We should have known...
> 1. the getChain function could easily have been overloaded such that the old
> and new versions could co-exist.
True. Somehow I was thinking that we could only have BC by adding the name as
the last (and optional) argument of the joint, but that would require you to
specify all the arguments when you use the joint. The last few arguments of the
joint are not used very often (inertia etc), so that seemed like a pain.
But ya, you're right, why not just add another constructor that takes the name
as its first argument. BC with one line of code!
> 2. is just spitting in the wind.
That would have been very unpleasant.
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
Peter Soetens <peter [..] ...> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |peter [..] ...
--- Comment #9 from Peter Soetens <peter [..] ...> 2009-07-23 00:11:15 ---
(In reply to comment #5)
> Ok, I like this patch very much, but it will make all KDL-related code break.
> Since we have a released version now (1.0.0), i can however commit it on trunk,
> unless someone has some serious objections?
Guess who's reading your email :-)
Not withstanding the need/quality of Wim's patch, you need to consider this:
1. the getChain function could easily have been overloaded such that the old
and new versions could co-exist. Both have a different number of arguments, so
the compiler could figure out which one the user wants -> +1 for backwards
compatibility.
2. Breaking API drastically for adding something simple as a name for an object
is just spitting in the wind. You could have done this equally backwards
compatible, where the 'old version' uses empty names and a 'new' getChain (or
other functions relying on unique names) would fail on nameless chains.
Not doing that is how you get yourself from 1.0.0 to 2.0.0. Keeping BC is a
simple 1.0 -> 1.1 upgrade. The patch is not worth the major version change
imho,
*even* if all users will use the new version.
It's a lesson in version control.
Peter
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
--- Comment #8 from Wim Meeussen <meeussen [..] ...> 2009-07-22 17:22:20 ---
Created an attachment (id=480)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=480)
python bindings
Welcome back! Alexis Maldonado provided a patch to get the python bindings
work with the new segment/joint types.
Alexis wrote:
"
This patch:
- repairs the bindings (change to eigen, names of joints, ...)
- makes kdl look for Eigen2 also in system directories
- fixes a typo in the include directory for tinyxml
- in the manifest file, adds the python path
Note: This patch adds a new file: ( src/bindings/python/std_string.sip ),
"
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
--- Comment #7 from Ruben Smits <ruben [dot] smits [..] ...> 2009-07-22 16:04:05 ---
Created an attachment (id=478)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=478)
also updates bindings for this change
Updates the RTT and python bindings and the python bindings tests for the
proposed adding of names to the joint and segment classes
Ruben
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
--- Comment #6 from Ruben Smits <ruben [dot] smits [..] ...> 2009-07-22 15:19:11 ---
You did not supply changes for the rtt/python bindings.
I'm figuring those out right now.
Ruben
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
--- Comment #5 from Ruben Smits <ruben [dot] smits [..] ...> 2009-07-22 14:30:58 ---
Ok, I like this patch very much, but it will make all KDL-related code break.
Since we have a released version now (1.0.0), i can however commit it on trunk,
unless someone has some serious objections?
Ruben
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
--- Comment #4 from Wim Meeussen <meeussen [..] ...> 2009-07-01 19:30:49 ---
Created an attachment (id=464)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=464)
New patch that includes name in Segment and Joint
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
--- Comment #3 from Wim Meeussen <meeussen [..] ...> 2009-07-01 19:30:17 ---
> Maybe we can change the Segment to store its name? This way names are both in
> the Tree and Chain class, and it doesn't add any overhead for the solvers.
I do like this idea!
I prepared a patch that adds a name to both the Segment and the Joint. This
makes adding segments/chains/trees to a tree nicer. It also allowed us to get
rid of a _lot_ of hacks in our code that uses kdl: similar to the getChain
function returning a vector of segment names, we had mappings between joint
names and segment names stores in various places because kdl did not store this
information. I also updated the regression tests because the patch does not
maintain backwards compatibility: the constructor now requires a name. I didn't
make the name an optional argument, because the name is actually used in the
tree object to check where and if segments can be added.
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
Ruben Smits <ruben [dot] smits [..] ...> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |ruben [dot] smits [..] ...en.b
| |e
--- Comment #2 from Ruben Smits <ruben [dot] smits [..] ...> 2009-07-01 08:55:24 ---
(In reply to comment #0)
> I wanted to propose a patch for the getChain function on a kdl Tree. The patch
> changes 2 things:
> 1) the getChain function returns true/false to indicate success or failure.
> This boils down to a check if a chain between the root name and tip name
> exists.
We definitely need this.
> 2) the getChain function returns a vector of all the segment names in the newly
> constructed chain. In a kdl Tree all segments have a name, but in a Chain this
> is not the case. So, when you extract a chain from a tree, this information is
> lost. Therefore it is useful to know which segments are actually in the chain
> that is returned.
Maybe we can change the Segment to store its name? This way names are both in
the Tree and Chain class, and it doesn't add any overhead for the solvers.
Ruben
[Bug 682] Updated getChain function
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=682
--- Comment #1 from Wim Meeussen <meeussen [..] ...> 2009-06-30 18:06:00 ---
Created an attachment (id=463)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=463)
Proposed patch