issue #47 change geometry type

[smgee]

Addresses issue #47 - user can change the geometry type and geometry properties for a link's visual and collision property.

[gavanderhoorn]

Thanks for the PR.

Can you describe your approach a bit? There is quite some duplicated code introduced in onValueChanged(..), and I'm wondering if it would've been possible to refactor LinkProperty in such a way that changing the geometry type (and fixing up the missing parts in the model) and then retriggering / recreating the properties by making use of what is already there would do the trick. It seems (but it's very likely I'm missing something) like that would fix #47 without too many changes.

[smgee]

Sure. This was a bit of a two-step approach. The first step was changing the geometry type in the property widget and populate the widget with the appropriate attributes i.e. radius for sphere, etc. The second step was to make sure the model then pointed to the proper geometry object.

When the GeometrySharedPtr was changed from say, a mesh type to a sphere type then that new GeometrySharedPtr now has to be tracked by the model, which is where you see the void on_propertyWidget_visualGeometryChanged(LinkProperty *property) and void on_propertyWidget_collisionGeometryChanged(LinkProperty *property) in URDFProperty. The passthrough slot in LinkProperty then emits a signal to URDFProperty, which is definitely not my favorite approach, but I didn't see any reference to the URDFProperty parent in LinkProperty so I couldn't attach the lower LinkCollisionProperty or LinkVisualProperty signal directly to URDFProperty.

I do see these connections up to URDFProperty necessary from the level that they are at because of scope issues. However, as I look at it now, it might be possible to be refactored from URDFProperty to connect the LinkProperty->LinkVisualProperty->LinkGeometryProperty signal directly to the slots in URDFProperty and if that is a cleaner approach I can investigate this.

I didn't want to change the structure too much from it's current form and that's why you see a separate signal for each visual geometry and collision geometry, but another approach could be handled in one slot in URDFProperty that differentiates using a char type as a parameter from a refactored LinkProperty.

[smgee]

I've gone back and done some testing to see if I can find a cleaner solution and I believe that your suggestion of refactoring from LinkProperty is a good solution and cleaner than the current implementation. From the onChildValueChange slot in LinkProperty I can change the geometry type of the link directly instead of needing to do that from URDFProperty. Then through getters and setters can update the GeometrySharedPtr down in LinkGeometryProperty without creating a new LinkGeometryProperty as that causes seg-faults. However, I don't see a good way to differentiate between a visual geometry signal and a collision geometry signal within LinkProperty::onChildValueChange without using two different slots, such as LinkProperty::onVisualChildValueChange and LinkProperty::onCollisionChildValueChange or similar.

Would this be acceptable?

If we find this new implementation acceptable - what is the process? Does this PR get rejected and I create a new one or do I simply commit and push to the forked traclabs:feature/issue47-merged which this PR is based off of?

[gavanderhoorn]

You can just push to the same branch. This will update the PR accordingly.

I'm having a bit of a hard time understanding what you're doing now exactly from your description, but I'll take a look if you can push your updates.

Thanks for revisiting it 👍.

[gavanderhoorn]

@smgee: I ended up merging your implementation instead of the one in #96: yours was the first one and it is working nicely. Thanks for taking a second shot at implementing it, appreciated. I squashed the two sets of changes in this PR into two commits to preserve the fact that you first tried a different approach. I didn't think all the fixup commits were really important to keep around, but other than that it's all merged.

I've just pushed the Milestone 1 merge candidate that includes this PR, see m1_merge_candidate (diff here).

It contains all changes proposed in this PR (plus some additional ones), so once m1_merge_candidate is merged, we can close this PR as merged as well.

[gavanderhoorn]

Could I ask you to verify that the merge candidate works as expected for you?

[gavanderhoorn]

Now part of m1_merge_candidate_v2, so closing this.

[gavanderhoorn]

Thanks @smgee, this worked nicely. 👍.

Parts have been changed in #96, but it's still part of the application.