Skip to content
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

Fixes an error in the OBB orientation calculation #100

Merged
merged 1 commit into from
Jun 2, 2018

Conversation

curds01
Copy link
Collaborator

@curds01 curds01 commented May 31, 2018

The documentation of getXBasis() and getYBasis() did not agree with what
was actually returned. This led to an incorrect result in computing the OBB
centroid.

  1. This elaborates on the documentation and how the basis vectors should be used.
  2. It also corrects usage in OBBShape::setDirections and OBBShape::getTargetPoint

Note to reviewer: some lines appear changed because whitespace has changed (tabs swapped for spaces -- sorry about that.)

Fixes #99


This change is Reviewable

@MengeCrowdSim
Copy link
Owner

@alafi I think this should fix your issue. Wanna take a look?

@alafi
Copy link

alafi commented May 31, 2018

The fix seems to be flipping the OBB (or the rendering is erroneous?).
As per the documentation for the OBB, a positive angle causes counter-clockwise rotation. This is not the case after applying the fix.

Check the images below from the randomGoal example.
Quote from the behavior file:
<Goal type="OBB" id="1" x="-1" y="5" width="2" height="2" angle="10.0" weight="1.0" />

image
image

Copy link
Collaborator Author

@curds01 curds01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the whole "The-OBB-is-rotating-the-wrong-way-thing"

pivot) and expressed in the geometry frame as:
`r_GQ = [(r_WQ - r_WG) * Bx, (r_WQ - r_WG) * By]^T` or
`r_GQ = R_GW * (r_WQ - r_WG)`.
Functionality that becomes:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Functionally, that becomes:"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Vector2 r_WQ_W;
Vector2 Bx = getXBasis();
Vector2 By = getYBasis();
Vector2 r_GQ_W = r_WQ - pivot_;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be clearer if the definitions of Bx and By came after r_GQ_W. (Same for getYBasis().)

@curds01
Copy link
Collaborator Author

curds01 commented May 31, 2018

Blast! I was so focused on making sure that the numbers were internally consistent, that I didn't look for that. Great catch. I'll address it and ping you again.

@MengeCrowdSim
Copy link
Owner

This is what I see. Did you pull the branch onto an older version of master?

capture

@MengeCrowdSim
Copy link
Owner

MengeCrowdSim commented Jun 1, 2018

Ah...the difference is between release and debug. So, release looks right, debug looks wrong...

Furthermore, it's clear that it's purely a rendering artifact. As I select different agents, their "near" goals move diagonally up to the left (even though the visualization of the goal moves up to the right). So, I'll take a look at the vis and figure out what's going on.

capture

@curds01 curds01 force-pushed the PR_fix_obb_basis branch 2 times, most recently from 0560633 to ceb7ee9 Compare June 1, 2018 02:57
@curds01
Copy link
Collaborator Author

curds01 commented Jun 1, 2018

Ok, I've updated the commit and I believe this fixes things.

(Also the release vs debug thing may not have been 100% accurate. There's a vague chance that VisualStudio didn't update everything it should've. However, this feels good.)

Moral of the story: I desperately need to get unit testing into Menge. A HUGE glaring hole...

@curds01 curds01 force-pushed the PR_fix_obb_basis branch from ceb7ee9 to 5506171 Compare June 1, 2018 03:01
@alafi
Copy link

alafi commented Jun 1, 2018

OK, this seems like fixed it.

However, I am still getting the following assertion failed every while and then:
Assertion failed: det( right, preferred )>= SPAN_EPS, file ..\..\..\src\Menge\mengeCore\Agents\PrefVelocity.cpp, line 106

Here are the parameter values for one of the incidents:
right = {_x=-0.573575854 _y=0.819152474 }
preferred = {_x=1.00000000 _y=0.000000000 }

The exception is thrown when calling PrefVelocity::setSpan in OBBShape::setDirections so it might be related to this issue. BTW it used to happen before and still happening after the fix.

Thank you for the great work and your follow up.

@curds01
Copy link
Collaborator Author

curds01 commented Jun 1, 2018

Without looking to the code, I suspect I know the problem. The vector [1, 0] is most likely the default result when normalizing a zero vector. Obviously, in this case, that's a bad thing. I think I know where to look for that and I'll tackle it this evening. Once I look at it, I may include it in this PR, or I may PR it separately (the separate being a case of suspecting that there are mostly likely multiple instances of that across all the goals....)

@MengeCrowdSim
Copy link
Owner

Review status: all files reviewed at latest revision, all discussions resolved.


releaseNotes.txt, line 3 at r1 (raw file):

----------------------------------------------------------------
Release 0.9.3
	???? ??, 2018

FYI spaces instead of tabs


src/Menge/MengeCore/Math/Geometry2D.h, line 777 at r1 (raw file):

      @brief    Returns the x-axis of the OBB's local frame expressed in the world frame.

      If we say that Bx = getXBasis() and By = getYBasis(), are *column* vectors, we can define the 2x2

BTW (Meta) Line too long


Comments from Reviewable

@curds01 curds01 force-pushed the PR_fix_obb_basis branch from 5506171 to 65740e7 Compare June 2, 2018 02:46
The documentation of getXBasis() and getYBasis() did *not* agree with what
was actually returned. This led to an incorrect result in computing the OBB
centroid.

1. This elaborates on the documentation and how the basis vectors should
   be used.
2. It also corrects usage in OBBShape::setDirections and
   OBBShape::getTargetPoint
3. Modifies rendering to match.
4. Adds two methods to the OBBShape -- convertToWorld() and
   convertToGeometry() to remove the burden of callers on how to transform.
@curds01 curds01 force-pushed the PR_fix_obb_basis branch from 65740e7 to a986d03 Compare June 2, 2018 02:47
@MengeCrowdSim
Copy link
Owner

:LGTM:


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@MengeCrowdSim MengeCrowdSim merged commit 9702d34 into MengeCrowdSim:master Jun 2, 2018
@curds01 curds01 deleted the PR_fix_obb_basis branch June 2, 2018 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants