DATA 498 add labels to geometries in RDK #1438
Conversation
| @@ -218,7 +218,7 @@ func (jpcs *joinPointCloudSource) NextPointCloudNaive(ctx context.Context) (poin | |||
| savedDualQuat := spatialmath.NewZeroPose() | |||
| pcSrc.Iterate(numLoops, loop, func(p r3.Vector, d pointcloud.Data) bool { | |||
| if jpcs.sourceNames[iCopy] != jpcs.targetName { | |||
| spatialmath.ResetPoseDQTransalation(savedDualQuat, p) | |||
| spatialmath.ResetPoseDQTranslation(savedDualQuat, p) | |||
There was a problem hiding this comment.
nit: typo in "Transalation"
| @@ -62,7 +62,7 @@ type Data interface { | |||
| // Note(erd): we should try to remove this in favor of immutability. | |||
| SetValue(v int) Data | |||
|
|
|||
| // Value returns the intesity value, or 0 if it doesn't exist | |||
| // Intensity returns the intensity value, or 0 if it doesn't exist | |||
There was a problem hiding this comment.
nit: The function name is "Intensity" not "Value".
| test.That(t, err, test.ShouldBeNil) | ||
| box, err := BoundingBoxFromPointCloud(c.pc) | ||
| box, err := BoundingBoxFromPointCloudWithLabel(c.pc, "box") |
There was a problem hiding this comment.
Updated this test to test the new BoundingBoxFromPointCloudWithLabel function, which is actually the old function's logic with a new function signature (it takes in a label now). It seemed liked the simplest way to cover the new function, BoundingBoxFromPointCloudWithLabel. See GitHub comment above this function for more info.
| } | ||
|
|
||
| // ToProto converts the box to a Geometry proto message. | ||
| // ToProtobuf converts the box to a Geometry proto message. |
There was a problem hiding this comment.
nit: Function name is "ToProtobuf" not "ToProto"
| @@ -118,7 +126,6 @@ func (b *box) CollidesWith(g Geometry) (bool, error) { | |||
| return true, newCollisionTypeUnsupportedError(b, g) | |||
| } | |||
|
|
|||
| // CollidesWith checks if the given box collides with the given geometry and returns true if it does. | |||
There was a problem hiding this comment.
nit: This comment is a duplicate of the comment above and isn't actually for the DistanceFrom function; it's for the CollidesWith function.
| return creator, nil | ||
| } | ||
| // never try to infer point geometry if nothing is specified | ||
| } | ||
| return nil, fmt.Errorf("%w %s", ErrGeometryTypeUnsupported, string(config.Type)) | ||
| } | ||
|
|
||
| // NewGeometryFromProto instatiates a new Geometry from a protobuf Geometry message. | ||
| // NewGeometryFromProto instantiates a new Geometry from a protobuf Geometry message. |
| @@ -51,6 +51,7 @@ func TestGeometrySerialization(t *testing.T) { | |||
| newVc, err := config.ParseConfig() | |||
| test.That(t, err, test.ShouldBeNil) | |||
| test.That(t, gc.NewGeometry(pose).AlmostEqual(newVc.NewGeometry(pose)), test.ShouldBeTrue) | |||
| test.That(t, config.Label, test.ShouldEqual, testCase.name) | |||
There was a problem hiding this comment.
To simply test the label was set correctly I made it the same name as the testCase's name iff the testCase can be parsed, i.e., success == true.
| @@ -15,7 +15,7 @@ type Orientation interface { | |||
| RotationMatrix() *RotationMatrix | |||
| } | |||
|
|
|||
| // NewZeroOrientation returns an orientatation which signifies no rotation. | |||
| // NewZeroOrientation returns an orientation which signifies no rotation. | |||
| @@ -15,7 +15,7 @@ import ( | |||
| ) | |||
|
|
|||
| // Epsilon represents the acceptable discrepancy between two floats | |||
| // representing spatial coordinates wherin the coordinates should be | |||
| // representing spatial coordinates wherein the coordinates should be | |||
| // ResetPoseDQTransalation takes a Pose that must be a dualQuaternion and reset's it's translation. | ||
| func ResetPoseDQTransalation(p Pose, v r3.Vector) { | ||
| // ResetPoseDQTranslation takes a Pose that must be a dualQuaternion and reset's it's translation. | ||
| func ResetPoseDQTranslation(p Pose, v r3.Vector) { |
There was a problem hiding this comment.
nit: typo in "Transalation"
|
@raybjork Hey Ray, since you worked on geometries, we are adding a label field to Geometries, so that people can select on Geometries by user-defined names as well. |
| func NewBoxCreator(dims r3.Vector, offset Pose, label string) (GeometryCreator, error) { | ||
| if dims.X <= 0 || dims.Y <= 0 || dims.Z <= 0 { | ||
| return nil, newBadGeometryDimensionsError(&box{}) | ||
| } | ||
| return &boxCreator{dims.Mul(0.5), pointCreator{offset}}, nil | ||
| return &boxCreator{dims.Mul(0.5), pointCreator{offset, label}, label}, nil | ||
| } | ||
|
|
||
| // NewGeometry instantiates a new box from a BoxCreator class. | ||
| func (bc *boxCreator) NewGeometry(pose Pose) Geometry { |
There was a problem hiding this comment.
@raybjork Do you think the new label argument should go in the signature of BoxCreator, or in the signature of NewGeometry?
There was a problem hiding this comment.
As of now it's in both, sort of. For NewGeometry it just grabs it from the *boxCreator. We could add it to NewGeometry too. We'd just have to be wary of tiebreaks in case it's passed in as an argument and *boxCreator already has one. In that case we'd probably want to the argument to take precedence. Right now users would have to update the label before calling NewGeometry, either in the call to NewBoxCreator or box.label.
There was a problem hiding this comment.
Yea, I wondering if it's only NewGeometry() that should pass the label to the box{}/sphere{}/etc struct, or if it should be xCreator as you currently have it. I'll want to wait to hear from @raybjork to see how they use these creators, and if it makes sense to set the label at the creator level.
There was a problem hiding this comment.
The GeometryCreator is the one that should have the label and provide it.
The way this information is used, is that any individual geometry is intended to be ephemeral and only valid at that location. The GeometryCreator persists the information about things like the dimensions of the geometry, and instantiates new versions of that geometry at various poses as requested.
There shouldn't be a situation where people are calling NewGeometry on a GeometryCreator and wanting it to have a different label, that should be an entirely new GeometryCreator.
There was a problem hiding this comment.
Excellent, then all the open questions are resolved on my end- LGTM @bazile-clyde
|
Oops, Ray isn't here this week, @biotinker, could you comment on where labels should be set: Whether when the Creator is made, or when NewGeometry is called? |
bazile-clyde
force-pushed
the
DATA-498-add-labels-to-geometries-in-rdk
branch
from
541f27c to
de47540
Compare
|
This commit is the first part of DATA-498. The next part will focus on the client and server interface. This seemed like a good stopping point since it's pretty large and self-contained.