Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Urdf: store handle to created Urdf object. #160

Merged
merged 1 commit into from
Jan 30, 2017

Conversation

achim-k
Copy link
Contributor

@achim-k achim-k commented Jan 25, 2017

When having multiple URDFs visualized, I was missing the option to remove a specific URDF instance. This PR adds the option to assign a name to the URDF object, so it can be easily found in the viewer's scene objects, e.g. for removal:

viewer.scene.children = viewer.scene.children.filter(function(node) {
  return !(node instanceof ROS3D.Urdf && node.name === "foo");
});

@sevenbitbyte
Copy link
Member

Why not use required data for identification such as simply keeping the options.path as a member variable? Other wise to copy this pattern consistently every ros3d object would need names. It seems a required piece of information like keeping a reference to your Urdf's in your application or looking them up by the path, which is required, are more universal solutions.

You can always attach additional fields, outside of the constructor, to the object after construction to support your specific application's needs.

@achim-k
Copy link
Contributor Author

achim-k commented Jan 27, 2017

Why not use required data for identification such as simply keeping the options.path as a member variable?

I have multiple URDFs which share the same options.path, but options.param would be unique in my case.

It seems a required piece of information like keeping a reference to your Urdf's in your application or looking them up by the path, which is required, are more universal solutions.

How would I get a reference to the ROS3D.Urdf instance when doing

// Setup the URDF client.
var urdfClient = new ROS3D.UrdfClient({
  ros : ros,
  tfClient : tfClient,
  path : 'http://resources.robotwebtools.org/',
  rootObject : viewer.scene,
  loader : ROS3D.COLLADA_LOADER_2
});

Is there a possibility to access the created ROS3D.Urdf instance through the urdfClient object?

You can always attach additional fields, outside of the constructor, to the object after construction to support your specific application's needs.

This only helps if I can get a reference to the created ROS3D.Urdf instance?

@sevenbitbyte
Copy link
Member

My suggestion would be to simply modify ROS3D.UrdfClient to keep a reference to the underlying ROS3D.Urdf as well as the param name. I would also like to see the tfPrefix and loader kept around for querying purposes.

Then in your application keep a reference to the client and grab your Urdf when needed. Use could be something like:

var urdfClient1 = new ROS3D.UrdfClient({
  ros : ros,
  tfClient : tfClient,
  path : 'http://resources.robotwebtools.org/',
  rootObject : viewer.scene,
  loader : ROS3D.COLLADA_LOADER_2
});

var urdfClient2 = new ROS3D.UrdfClient({
  ros : ros,
  tfClient : tfClient,
  path : 'http://resources.robotwebtools.org/',
  rootObject : viewer.scene,
  loader : ROS3D.COLLADA_LOADER_2
});

var urdfNameMap = {}
urdfNameMap['robot1'] = urdfClient1.urdf
urdfNameMap['robot2'] = urdfClient2.urdf

Or look up using the param like:

var urdfClients = [];
urdfClients.push(new ROS3D.UrdfClient({
  ros : ros,
  tfClient : tfClient,
  param: 'robot1_description',
  path : 'http://resources.robotwebtools.org/',
  rootObject : viewer.scene,
  loader : ROS3D.COLLADA_LOADER_2
}));

urdfClients.push(new ROS3D.UrdfClient({
  ros : ros,
  tfClient : tfClient,
  param: 'robot2_description',
  path : 'http://resources.robotwebtools.org/',
  rootObject : viewer.scene,
  loader : ROS3D.COLLADA_LOADER_2
}));

function getUrdfByParam(paramName){
  for(var client in urdfClients){
    if(client.param == paramName){ return client.urdf }
  }
  return undefined
}

By keeping all the options attached to the object we enable all developers to query however they need at a later time without needing to somehow generate unique names for every instance in the scene.

@achim-k achim-k changed the title Urdf: add option to set name of Urdf object. Urdf: store handle to created Urdf object. Jan 30, 2017
@achim-k
Copy link
Contributor Author

achim-k commented Jan 30, 2017

@sevenbitbyte thanks for your suggestions, works for me and is also cleaner 👍

@T045T T045T merged commit 295be22 into RobotWebTools:develop Jan 30, 2017
@viktorku
Copy link
Member

Merged without grunt build.. Maybe add this check to your Travis CI job or make it a pre-commit hook?

@T045T
Copy link
Contributor

T045T commented Jan 31, 2017

That's a result of merging via the web UI - I was merging a bunch of PRs and figured I'd push a fresh grunt build for all of them.
You're right though, adding it to the travis config is a good idea. Last I checked, pre-commit hooks can't just be part of the repo but need to be configured by the user, so that's not really an option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants