-
Notifications
You must be signed in to change notification settings - Fork 209
Add joint limit and inertial link data #299
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
base: master
Are you sure you want to change the base?
Conversation
gkjohnson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you! I've added a few comments. We'll want to update the URDFLink documentation in the README, as well.
| origin: { xyz: Vector3 | null, rpy: Vector3 | null } | null; | ||
| mass: Number | null; | ||
| inertia: { ixx: Number, ixy: Number, ixz: Number, iyy: Number, iyz: Number, izz: Number } | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't initialize these to null if they are not in the file. Lets use the default values listed in the specification.
|
|
||
| isURDFLink: true; | ||
| urdfNode: Element | null; | ||
| inertial: URDFInertial | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets make sure this is always present, as well.
| angle: Number; | ||
| jointValue: Number[]; | ||
| limit: { lower: Number, upper: Number }; // TODO: add more | ||
| limit: { lower: Number, upper: Number, effort: Number | null, velocity: Number | null }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment - lets use the default values.
| // file path is null if a package directory is not provided. | ||
| if (filePath !== null) { | ||
|
|
||
| group.meshPath = filePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the need for this field a bit further? And why do you need to resolved, absolute path rather than the local path provided in the URDF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially I use this as an identifier to figure when a particular link mesh (e.g. a referenced STL or DAE file) in the URDF has loaded, so that I can do operations on the geometry. I believe the loadAsync call resolves only when the URDF file has been loaded and parsed, however at that time the geometries referenced in the links still seem to be empty.
If there is another way to do this, please let me know so I can try using that approach instead of doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that the promise only resolves once the robot hierarchy is loaded, not once all the geometry is ready. You can use the LoadingManger to detect when the full model is loaded, though:
let robot;
const manager = new LoadingManager();
manager.onComplete = () => {
// everything in the robot model is loaded
};
const loader = new URDFLoader( manager );
loader.loadAsync( url ).then( result => {
robot = result;
} );Not the most ergonomic but it works, is easy to wrap, and will fire when all textures are loaded, as well. If you'd like something more built in we can talk about a flag that that can be toggled to await all the geometry loading.
You can also replace the mesh loader with one that will process the geometry when it's loaded:
loader.loadMeshCb = ( path, manager, onComplete ) => {
// load mesh, process it, call onComplete
}; | const rpy = origin ? origin.getAttribute('rpy') : null; | ||
| const mass = subNodes.find(sn => sn.nodeName.toLowerCase() === 'mass'); | ||
| const inertia = subNodes.find(sn => sn.nodeName.toLowerCase() === 'inertia'); | ||
| target.inertial = origin || mass || inertia ? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the other comments, lets make sure this inertial field is already fully initialized with the correct defaults. And then this block can just overwrite any fields that are present in the file.
| target.urdfNode = link; | ||
|
|
||
| // Extract the attributes | ||
| children.forEach(n => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow the pattern used elsewhere in the file:
const inertialNode = children.find(n => n.nodeName.toLowerCase() === 'inertial');
if ( inertialNode !== null ) {
[ ...inertialNode.children ].forEach( child => {
const type = n.nodeName.toLowerCase();
if ( type === 'origin' ) { /* ... */ }
else if ( type === 'mass' ) { /* ... */ }
// and so on...
} );
}|
Thanks for taking a look - I'll make these changes during the week! |
This PR adds the following:
inertialparsing support forlinks according to the specificationeffortandvelocitylimit support forjoints according to the specificationmeshPathvariable so downstream applications can track when all meshes are loaded if they want to (I didn't add this functionality due to not wanting to add additional dependencies, however this was the change I needed to do so).umdfiles