MengeCrowdSim / Menge

The source code for the Menge crowd simulation framework
Apache License 2.0
138 stars 64 forks source link

Fixes an error in the OBB orientation calculation #100

Closed curds01 closed 6 years ago

curds01 commented 6 years ago

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 commented 6 years ago

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

alafi commented 6 years ago

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

curds01 commented 6 years ago

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 commented 6 years ago

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

capture

MengeCrowdSim commented 6 years ago

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 commented 6 years ago

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...

alafi commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

MengeCrowdSim commented 6 years ago
:LGTM:

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


Comments from Reviewable