Adding default point color to SfmTrack to fix equality check
#659
Conversation
ayushbaid
added
bugfix
gtsam/slam/dataset.h
Outdated
| @@ -221,7 +221,7 @@ struct SfmTrack { | |||
| SfmTrack(): p(0,0,0) {} | |||
| SfmTrack(const gtsam::Point3& pt) : p(pt) {} | |||
There was a problem hiding this comment.
@varunagrawal @ayushbaid @dellaert perhaps we should do
SfmTrack(float r = 0, float g = 0, float b = 0): p(0,0,0), r(r), g(g), b(b) {}
SfmTrack(const gtsam::Point3& pt, float r = 0, float g = 0, float b = 0) : p(pt), r(r), g(g), b(b) {}so we can use the RGB values later in rendering?
I'm confused why the r, g, b attributes are float in the class, instead of uint8
There was a problem hiding this comment.
perhaps unsigned char is more appropriate?
There was a problem hiding this comment.
Actually keeping it float makes sense in case the values aren't in the 0-255 range.
There was a problem hiding this comment.
I would say it's probably better to rethink this a bit: what if the image is 12bit per channel? What if the input is grayscale? What are the implications of assuming 8bit RGB?
There was a problem hiding this comment.
totally fair, good points
There was a problem hiding this comment.
@ProfFan maybe we can discuss this a bit more -- I do think that 8bit is much more common for our use cases in GTSAM and GTSFM, and I also think that if the input is grayscale, they wouldn't use the RGB attributes in the first place. And I think this attribute is purely for 3d point cloud visualization purposes, and I don't think grayscale values would contribute meaningfully to a 3d visualization.
There was a problem hiding this comment.
Agree with John. As this is just used for visualization, we can just stick with float R, G, B values, normalized to [0, 1] range.
| SfmTrack(): p(0,0,0) {} | ||
| SfmTrack(const gtsam::Point3& pt) : p(pt) {} | ||
| SfmTrack(float r = 0, float g = 0, float b = 0): p(0,0,0), r(r), g(g), b(b) {} | ||
| SfmTrack(const gtsam::Point3& pt, float r = 0, float g = 0, float b = 0) : p(pt), r(r), g(g), b(b) {} |
There was a problem hiding this comment.
Formatting please. Also, a good convention is to have struct/class variables be something like r_ and then have the initializer list be r_(r). This is consistent with Google C++ style which we follow.
There was a problem hiding this comment.
I like the style suggestion, but this would involve renaming all of the variables in the class, since none adhere to that guideline. @dellaert would you like to see a big change like that?
There was a problem hiding this comment.
I would vote for it since due to the previous lack of getters, it won't be an API change, so we should be able to maintain backwards compatibility.
There was a problem hiding this comment.
Actually, for classes the convention is underscore, for structs it is not (and should not).
It is an API change as these fields are public.
There was a problem hiding this comment.
Ah you're right. My mistake.
| SfmTrack(): p(0,0,0) {} | ||
| SfmTrack(const gtsam::Point3& pt) : p(pt) {} | ||
| SfmTrack(float r = 0, float g = 0, float b = 0): p(0,0,0), r(r), g(g), b(b) {} | ||
| SfmTrack(const gtsam::Point3& pt, float r = 0, float g = 0, float b = 0) : p(pt), r(r), g(g), b(b) {} |
There was a problem hiding this comment.
Actually, for classes the convention is underscore, for structs it is not (and should not).
It is an API change as these fields are public.
johnwlambert
dismissed
varunagrawal’s stale review
keeping current variable names since struct not class
The
equalsmethod fails right now when we run bundle adjustment on the same input twice. I checked that all the 2D measurements and 3d point match, but the color comparison fails.Adding a default value fixes the comparison.