Implement add and removal for links and joints #93

Levi-Armstrong · 2016-04-27T23:41:39Z

This implements the right click option for adding and removing links. When links are added an removed joints are automatically added and removed to prevent have a malformed urdf. Also the rearranging of joints is controlled by the on_propertyWidget_jointParentLinkChanged SLOT.

Levi-Armstrong · 2016-04-28T06:46:48Z

@swhart115 since there was some overlap between our pull requests I went and merged yours with mine. I had to change things a bit to work with the changes I made so would you mind testing it out and make sure the functionality you were wanting is still there?

gavanderhoorn · 2016-04-28T14:13:14Z

@Levi-Armstrong: could you detail a bit where you made use of functionality added by @swhart115?

gavanderhoorn · 2016-04-28T14:17:07Z

This works quite well. I'm not sure always removing a joint when removing the child is something we'd want though: I can imagine a situation where I'd want to retain a 'complicated' transform, but just delete and re-add a child link in order to start over. Adding a link-joint pair however does make editing a nicer experience.

gavanderhoorn · 2016-04-28T14:18:51Z

I'm not sure this is a result of the changes by @Levi-Armstrong or if it was already present in #87, but dragging-and-dropping randomly made some of the links in my Fetch model disappear / attach to a random other link.

Perhaps something is not entirely correct in the mappings between TreeItems and links? Or the updates to those structures?

gavanderhoorn · 2016-04-28T14:20:46Z

urdf_editor/include/urdf_editor/urdf_property.h

    void addToTreeWidget(urdf::LinkSharedPtr link, QTreeWidgetItem* parent);
    void addToTreeWidget(urdf::JointSharedPtr joint, QTreeWidgetItem* parent);

-    void addModelLink(QTreeWidgetItem* parent);
+    urdf::LinkSharedPtr addModelLink(QTreeWidgetItem* parent);


All these addX and removeX are probably candidates for a round of refactoring. That's essentially what you already suggested @Levi-Armstrong, some time ago (ie: create a bunch of helpers to add/remove links/joints to a urdf data structure). Not sure how much priority we want to give that, but it would remove model-manipulation from the URDFProperty, which I think it is a good thing.

Are we still planning at some point to move to our own internal data structure? If so this would be a good place to for all of the these methods.

Yes, we'll have to move away from urdf eventually. For now a simple refactor to get these methods out of this class would be good enough.

gavanderhoorn · 2016-04-28T15:08:42Z

urdf_editor/src/urdf_property.cpp

+              }
+
+              // during the removal process the urdf is broken then fixed, so an update is required.
+              on_propertyWidget_valueChanged();


Should this be emitted? Not sure if you intentionally wanted to keep this local to this class.

Does emit work on SLOT's; I assumed it was only for SIGNALS?

gavanderhoorn · 2016-04-28T15:12:07Z

I've reviewed 4745792 and added comments through this PR, but overall this looks nice.

Do you think refactoring some of the QTreeWidgetItem re-shuffling code into methods could benefit @swhart115 in #87 (more than you did now I mean)?

swhart115 · 2016-04-28T15:50:36Z

@Levi-Armstrong I am testing the drag-and-drop functionality under this PR #93 . There seems to be one capability not carried over, and that's the ability to drag a link/subtree out of the chain (the two-roots case). There has been a lot of discussion on this given that it results in an invalid URDF, but also doesn't let the user change the root link after its created, as a result. @gavanderhoorn what is the call on this? Do we want to allow invalid, multi-root Link structures, or do we want to prevent this from happening?

swhart115 · 2016-04-28T15:52:59Z

Also, dragging and dropping of joints seems to be allowed, but my original PR prevented this from happening. Due to @Levi-Armstrong 's changes, it reorganizes the link tree accordingly. So i'm fine allowing this, if @gavanderhoorn is cool with it.

gavanderhoorn · 2016-04-28T15:59:21Z

@swhart115 wrote:

Also, dragging and dropping of joints seems to be allowed, but my original PR prevented this from happening. Due to @Levi-Armstrong 's changes, it reorganizes the link tree accordingly. So i'm fine allowing this, if @gavanderhoorn is cool with it.

I'm not sure how intuitive drag-and-drop for joints is: it's certainly something that is possible, but does it add anything? Links being a tree structure -- and then re-ordering the branches -- makes sense to me, but for joints I'm not sure. Can we come up with a use-case where re-ordering joints (in a tree!) makes sense / is easier than doing it with the links?

gavanderhoorn · 2016-04-28T16:02:44Z

@swhart115 wrote:

[..] There has been a lot of discussion on this given that it results in an invalid URDF, but also doesn't let the user change the root link after its created, as a result. @gavanderhoorn what is the call on this? Do we want to allow invalid, multi-root Link structures, or do we want to prevent this from happening?

I don't really have a preference for how, as long as we have a simple/intuitive way to rearrange a link tree such that we can change the root link.

swhart115 · 2016-04-28T16:02:46Z

@gavanderhoorn I've been advocating the removal of the tree structure for the joints for awhile, instead just providing them in a list under Joints.

That being said, it would be nice to have an easier way to see what joints are associated with a link other then manually clicking through each joint and reading what the parent and child is. I'd advocate adding new properties to the links that show the parent and (possibly multiple) children joints.

swhart115 · 2016-04-28T16:06:48Z

@gavanderhoorn

I don't really have a preference for how, as long as we have a simple/intuitive way to rearrange a link tree such that we can change the root link.

This is not the case right now. You are stuck with the initial root link. I will add the functionality back in as it was before the merge with #93

gavanderhoorn · 2016-04-28T16:06:51Z

If the joints are shown in a flat list, what about:

joint_1 (base_link ⇾ link_1)
joint_2 (link_1 ⇾ link_2)
..

or a similar approach?

Adding properties is possible, but a) implies they can be changed from there as well (I would not be in favour of that) and b) properties are invisible until a user clicks on the link.

gavanderhoorn · 2016-04-28T16:09:15Z

@swhart115 wrote:

@gavanderhoorn

I don't really have a preference for how, as long as we have a simple/intuitive way to rearrange a link tree such that we can change the root link.

This is not the case right now. You are stuck with the initial root link. I will add the functionality back in as it was before the merge with #93

Could you do that in a separate branch? So we can compare the two alternatives?

swhart115 · 2016-04-28T16:11:49Z

@gavanderhoorn wrote:

Adding properties is possible, but a) implies they can be changed from there as well (I would not be in favour of that) and b) properties are invisible until a user clicks on the link.

a) What if they are just uneditable properties? b) this is still better then having to go through and click on each joint to see if it has a particular link as its parent.

gavanderhoorn · 2016-04-28T16:19:52Z

@swhart115 wrote:

a) What if they are just uneditable properties? b) this is still better then having to go through and click on each joint to see if it has a particular link as its parent.

a) that would be ok, but all other properties are editable, so that could be counter-intuitive, b) I agree, hence my suggestion to show this in the tree itself.

Levi-Armstrong · 2016-04-30T06:50:30Z

@gavanderhoorn 👍

If the joints are shown in a flat list, what about:
joint_1 (base_link ⇾ link_1)
joint_2 (link_1 ⇾ link_2)
..

Levi-Armstrong · 2016-04-30T06:52:48Z

@gavanderhoorn I have started to refactor the mapping. Is the implementation for the mapping get methods what you looking for?

Levi-Armstrong · 2016-04-30T07:04:48Z

@gavanderhoorn: I do see a case where the user entered the wrong information for a joint and they need to reassign it to another link. In this case I could set it up so when the child link of the joint is changed it performs a swap between the this joint and the joint that is currently assigned to the selected child link. Would this be desirable behavior; I am just trying to prevent from breaking the urdf if possible? The other option would be to provide copy past functionality.

This works quite well. I'm not sure always removing a joint when removing the child is something we'd want though: I can imagine a situation where I'd want to retain a 'complicated' transform, but just delete and re-add a child link in order to start over. Adding a link-joint pair however does make editing a nicer experience.

gavanderhoorn · 2016-05-03T16:19:51Z

@Levi-Armstrong wrote:

@gavanderhoorn: I do see a case where the user entered the wrong information for a joint and they need to reassign it to another link. In this case I could set it up so when the child link of the joint is changed it performs a swap between the this joint and the joint that is currently assigned to the selected child link. Would this be desirable behavior; I am just trying to prevent from breaking the urdf if possible? The other option would be to provide copy past functionality.

hm. Not sure what the best approach here is. Swapping could lead to unintuitive situations (if the joint is swapped with something that is 'far away' in the model, parts of the geometry will suddenly be somewhere completely different from where they are 'now').

Let's leave the behaviour as it is right now. If not being able to delete a link without the joint is really a problem, we should ticket it and deal with it then.

gavanderhoorn · 2016-05-03T16:20:09Z

@Levi-Armstrong: now that you have opened #97, should this PR be closed?

Levi-Armstrong · 2016-05-04T06:46:57Z

Yes, I did not want to close it until you had a chance to review the other pull request.

swhart115 and others added 4 commits April 25, 2016 18:25

merged local Issue26 branch with latest ROS-I indigio-devel

1265de1

fixed to handle dragging to root link

d077e8e

undid changes to launch

c1e3ce6

Implement add and removal for links and joints

4745792

Levi-Armstrong mentioned this pull request Apr 28, 2016

Issue #26 link tree drag-drop #87

Closed

Levi Armstrong added 2 commits April 28, 2016 01:25

Merge pull request #87 joint and link tree drag-drop

0d00511

remove joint_property.cpp.org file

16e9cf9

gavanderhoorn reviewed Apr 28, 2016
View reviewed changes

Phase I changes

9c130f8

Levi-Armstrong mentioned this pull request May 2, 2016

Restructure code to remove the need for multiple maps. #97

Closed

Levi-Armstrong closed this May 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement add and removal for links and joints #93

Implement add and removal for links and joints #93

Levi-Armstrong commented Apr 27, 2016

Levi-Armstrong commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

gavanderhoorn Apr 28, 2016 •

edited

Loading

Levi-Armstrong Apr 28, 2016

gavanderhoorn Apr 28, 2016

gavanderhoorn Apr 28, 2016

Levi-Armstrong Apr 30, 2016

gavanderhoorn commented Apr 28, 2016

swhart115 commented Apr 28, 2016

swhart115 commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

swhart115 commented Apr 28, 2016 •

edited

Loading

swhart115 commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016 •

edited

Loading

gavanderhoorn commented Apr 28, 2016

swhart115 commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

Levi-Armstrong commented Apr 30, 2016

Levi-Armstrong commented Apr 30, 2016

Levi-Armstrong commented Apr 30, 2016

gavanderhoorn commented May 3, 2016

gavanderhoorn commented May 3, 2016

Levi-Armstrong commented May 4, 2016

Implement add and removal for links and joints #93

Implement add and removal for links and joints #93

Conversation

Levi-Armstrong commented Apr 27, 2016

Levi-Armstrong commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

gavanderhoorn Apr 28, 2016 • edited Loading

Choose a reason for hiding this comment

Levi-Armstrong Apr 28, 2016

Choose a reason for hiding this comment

gavanderhoorn Apr 28, 2016

Choose a reason for hiding this comment

gavanderhoorn Apr 28, 2016

Choose a reason for hiding this comment

Levi-Armstrong Apr 30, 2016

Choose a reason for hiding this comment

gavanderhoorn commented Apr 28, 2016

swhart115 commented Apr 28, 2016

swhart115 commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

swhart115 commented Apr 28, 2016 • edited Loading

swhart115 commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016 • edited Loading

gavanderhoorn commented Apr 28, 2016

swhart115 commented Apr 28, 2016

gavanderhoorn commented Apr 28, 2016

Levi-Armstrong commented Apr 30, 2016

Levi-Armstrong commented Apr 30, 2016

Levi-Armstrong commented Apr 30, 2016

gavanderhoorn commented May 3, 2016

gavanderhoorn commented May 3, 2016

Levi-Armstrong commented May 4, 2016

gavanderhoorn Apr 28, 2016 •

edited

Loading

swhart115 commented Apr 28, 2016 •

edited

Loading

gavanderhoorn commented Apr 28, 2016 •

edited

Loading