search

ros-industrial-attic / CAD-to-ROS

GUI tools for ROS setup files starting with URDFs (Unmaintained)
Apache License 2.0
12 stars 9 forks source link

Changes to link.geometry.origin don't propagate to model #92

Closed gavanderhoorn closed 8 years ago

gavanderhoorn commented 8 years ago

Changing xyz or rpy of an OriginProperty of a link in the browser doesn't seem to trigger an update of the in-memory model if the OriginProperty was added through the context menu.

Changes to the origin of a link.geometry loaded from disk do get propagated (ie: visualisation is updated), so it's likely that the newly added OriginPropertys don't get hooked up correctly to the change notification signals/slots.

Levi-Armstrong commented 8 years ago

@gavanderhoorn: It looks like when the right click option was added the connection to on value changed were not setup as they are here.

Also this is not only a problem for the OriginProperty but for all properties that can be added through the right click option since none of them setup the connection for when a value is changed.

gavanderhoorn commented 8 years ago

Thanks for looking into it. Should not be too hard to fix then.

phvass commented 8 years ago

Maybe @ShawnSchaerer could look at this?

ShawnSchaerer commented 8 years ago

Yes, I can

On Thu, May 12, 2016 at 9:15 AM, Paul Hvass notifications@github.com wrote:

Maybe @ShawnSchaerer https://github.com/ShawnSchaerer could look at this?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/ros-industrial-consortium/CAD-to-ROS/issues/92#issuecomment-218770389

gavanderhoorn commented 8 years ago

suggestion: try to avoid duplicating code for setting up the signals and slots. Refactor whatever is needed from the ctor (in some cases) to a separate method, then call that method (with suitable arguments) in the ctor and where-ever else it is needed.

ShawnSchaerer commented 8 years ago

I just added a new link an inertia and visual property. Next I added an origin property and then proceeded to change the value. the changed values propagate and are saved to the URDF file. I am not sure what is not working? @Levi-Armstrong do you have an example?

Levi-Armstrong commented 8 years ago

@ShawnSchaerer if you are working off one of the m1_merge_candidate branches this has been fixed for the link items by @jrgnicho but not for the joint.

When a joint property is loaded for each of the properties they are connected as shown here, but when a new property is added to an existing joint the connection is not made as shown here. Just a note, each property now has its own connection slot. Hope this helps.

ShawnSchaerer commented 8 years ago

I just checked and its working for an origin on the joint that I added to a brand new URDF. Seems to be working

Levi-Armstrong commented 8 years ago

Oh, After reviewing the code the connection is only used to trigger the reloading of the model. So without the connection if you add the origin through the context menu and change the value you would not see the changes in the visualizer. I am not sure why the data was not propagating down for @gavanderhoorn but it appears to be doing it now.

ShawnSchaerer commented 8 years ago

ok, I suggest we close this issue

On Wed, May 18, 2016 at 10:03 AM, Levi Armstrong notifications@github.com wrote:

Oh, After reviewing the code the connection is only used to trigger the reloading of the model. So without the connection if you add the origin through the context menu and change the value you would not see the changes in the visualizer. I am not sure why the data was not propagating down for @gavanderhoorn https://github.com/gavanderhoorn but it appears to be doing it now.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/ros-industrial-consortium/CAD-to-ROS/issues/92#issuecomment-220055175

gavanderhoorn commented 8 years ago

It could well be that what I reported in the OP has already been fixed. We'll have to test to make sure.

Levi-Armstrong commented 8 years ago

The connection issue should still be addressed, do you want to create a new issue related to this and close this one?

ShawnSchaerer commented 8 years ago

new issue

gavanderhoorn commented 8 years ago

If I load a urdf from disk, then proceed to add a link (anywhere), open the joint tree, drill down to the joint that connects the newly added link to the tree and try to edit any of the properties of the joint, those changes do not get propagated.

Changing the joint type fi (from unknown to fixed or something else) does not trigger a reload. Only after changing something in the link does the change in joint type get picked up.

Levi-Armstrong commented 8 years ago

When you are referring to a reload, are you talking about the reloading of the urdf within the rviz visualizer?

I followed your example and then save the urdf to a file and the new link and joint are present with the correct values.

If you were expecting the joint type change to cause a redraw of the robot, this will no logger happen. With the newly added signals that provide finer detail on what is changed, only items that affect the visual representation of the robot will trigger the reload. Now the link currently does not have this finer detail on what is changed so any change to the link will cause a redraw of the robot.

gavanderhoorn commented 8 years ago

If you were expecting the joint type change to cause a redraw of the robot, this will no logger happen.

I would expect any change that could influence the visual representation of the model to trigger a reload. Changing joint types would be such a change. If I add a joint it starts as an unknown type (#107), so the robot will disappear from the visualiser as the model is no longer correct. I would expect to be able to drill down to the offending joint, change its type and would expect the robot to reappear. IIRC, that doesn't happen right now.

I'm happy with the addition of the more fine grained signals, but I would think that what I described should be the implemented behaviour.

Levi-Armstrong commented 8 years ago

OK, I see now. To fix the issue should the joint type list be changed to not include Unknown since it is not even a possibility per the joint documentation?

gavanderhoorn commented 8 years ago

Shouldn't changing the joint type result in a re-parse of the model in any case? Fixing #107 is good, but that is a different issue imo.

Levi-Armstrong commented 8 years ago

The only reason to re-parse is to update the visualization and this change does not affect the visual. I understand currently that if there is an error in the re-parsing of the urdf the model disappears but this should be controlled by the UI so this does not occur. I am ok with making the change so the visualization is updated on any joint change to solve the issue. I think when we move from relying on the loading of the urdf to update the visual to where the editor is loading the visual geometries would solve a lot of these issues.

gavanderhoorn commented 8 years ago

The only reason to re-parse is to update the visualization and this change does not affect the visual.

make sense. Agreed.

We should still make sure other changes that do affect the visualisation trigger a refresh then.

Levi-Armstrong commented 8 years ago

OK, Should I submit the PR against this issue?

gavanderhoorn commented 8 years ago

If you fix the "joints should not be of unknown type" then it'd be related to #107, so not this issue.

We can close this one after we make sure all other changes that influence the visual aspects of the model do trigger a refresh.

Levi-Armstrong commented 8 years ago

@gavanderhoorn I should have something for you to review tomorrow on this issue.

gavanderhoorn commented 8 years ago

Fixed in #111.