DATA-855 Update model json files for grippers #1638
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
the
safe to test
nfranczak
changed the title
nfranczak
changed the title
raybjork
approved these changes
Dec 5, 2022
biotinker
requested changes
Dec 6, 2022
There was a problem hiding this comment.
Re-posting this comment as actual review feedback.
Rather than adding a geometry to the fake gripper, let's remove its model file entirely; that way, users can more easily use it to model any arbitrary gripper.
It only has a model file in the first place, due to some now-obsolete testing that was done back in 2021.
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
biotinker
reviewed
Dec 6, 2022
components/gripper/fake/gripper.go
Outdated
|
|
||
| var g gripper.LocalGripper = &Gripper{Name: config.Name, model: model} | ||
| var g gripper.LocalGripper = &Gripper{Name: config.Name, model: nil} |
There was a problem hiding this comment.
This can just be &Gripper{Name: config.Name}, no need for an explicit model: nil
There was a problem hiding this comment.
gotcha, will change now
There was a problem hiding this comment.
ah, I do not think that is allowed. I am getting:
cannot use &(Gripper literal) (value of type *Gripper) as referenceframe.Model value in struct literal: *Gripper does not implement referenceframe.Model (missing method AlmostEquals)compilerInvalidIfaceAssign
There was a problem hiding this comment.
Is that from a build or a test?
Also; we should actually be removing the model from the Gripper struct entirely for fake, and returning nil explicitly like in other grippers. Example from the softrobotics gripper:
// ModelFrame is unimplemented for softGripper.
func (g *softGripper) ModelFrame() referenceframe.Model {
return nil
}
There was a problem hiding this comment.
The error showed up right when I pasted &Gripper{Name: config.Name} into the model field.
Not an issue anymore as I removed the model field from the fake gripper struct per your request.
viambot
added
safe to test
biotinker
approved these changes
Dec 7, 2022
viambot
added
safe to test
viambot
removed
the
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
|
Note: merge base coverage results not available, comparing against closest 556fc19~1=(f75a4c4) instead
|
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates model json files for VG1 and fake gripper.
The geometry for fake gripper is the same as the geometry for VG1.