-
Notifications
You must be signed in to change notification settings - Fork 402
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
Extend bindings for OBB and misc fixes. #493
Conversation
Codecov Report
@@ Coverage Diff @@
## master #493 +/- ##
==========================================
- Coverage 59.19% 59.16% -0.04%
==========================================
Files 165 165
Lines 7340 7346 +6
Branches 84 84
==========================================
+ Hits 4345 4346 +1
- Misses 2995 3000 +5
Continue to review full report at Codecov.
|
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.
LGTM. One request to use constexpr
Co-Authored-By: Erik Wijmans <etw@gatech.edu>
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.
@Skylion007 thank you for all the courage to fix low level hard to capture issues. Well done!
@@ -107,7 +107,7 @@ OBB computeGravityAlignedMOBB(const vec3f& gravity, | |||
const float dd = d0[0] * d1[1] - d0[1] * d1[0]; | |||
|
|||
const float dx = s1[0] - s0[0]; | |||
const float dy = s1[1] - s0[0]; | |||
const float dy = s1[1] - s0[1]; |
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.
@Skylion007, can you remind me what was the bug here?
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.
dy was not actually calculating a delta y.
@@ -240,7 +240,7 @@ OBB computeGravityAlignedMOBB(const vec3f& gravity, | |||
aabb.extend(T_w2b * pt); | |||
} | |||
|
|||
return OBB{aabb.center(), aabb.sizes(), T_w2b.inverse()}; | |||
return OBB{T_w2b.inverse() * aabb.center(), aabb.sizes(), T_w2b.inverse()}; |
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.
And here.
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.
The center was not in world coordinates
@@ -39,13 +39,12 @@ std::vector<vec2f> convexHull2D(const std::vector<vec2f>& points) { | |||
} | |||
|
|||
// Build upper hull | |||
for (size_t i = idx.size() - 2, t = k + 1; i >= 0; i--) { |
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.
And here, just to have a history record for all this deep changes.
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.
Resolves a segfault that occurs on Linux due to undefined behavior of unsigned integer underflows.
* OBB fixes and extends. Also added error val to sample navigable point * Make getRandomNavigablePoint failure value constant. Co-Authored-By: Erik Wijmans <etw@gatech.edu> Co-authored-by: Erik Wijmans <ewijmans2@gmail.com>
* Extract ckpt_id from ckpt_path * rename util function * Move defrost
Motivation and Context
Added some functionality needed for properly using OBBs in the Habitat-API.
1). Add new python bindings to OBB
2). Fix bugs computeMOBB and expose to python.
3). Have sampleNavigablePoint return a point that will always not be navigable if it fails. That ensure that we get a proper error state that we can check against pt(inf,inf,inf).
How Has This Been Tested
It has been tested by being used for generating the ObjectNav datasets in the Habitat-API.
Types of changes
Checklist