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

[Cleanup] Cleaned up the collcabrate calculation #757

Merged
merged 3 commits into from Feb 9, 2016

Conversation

Projects
None yet
3 participants
@ulteq
Contributor

ulteq commented Feb 8, 2016

One vehicle standing, one vehicle driving at 80 km/h. The screenshots show the state after the collision.

Without this patch:

With this patch:

@mikadou

This comment has been minimized.

Show comment
Hide comment
@mikadou

mikadou Feb 9, 2016

Contributor

What is collcabrate? collision-cab-rate? Are cabs triangles/faces of the submesh? How are rate and distance in the collcabrate_t structure related?

Contributor

mikadou commented Feb 9, 2016

What is collcabrate? collision-cab-rate? Are cabs triangles/faces of the submesh? How are rate and distance in the collcabrate_t structure related?

@mikadou

This comment has been minimized.

Show comment
Hide comment
@mikadou

mikadou Feb 9, 2016

`Why was the if-block above moved into the for-loop?

mikadou commented on source/main/physics/Beam.cpp in 2bded4f Feb 9, 2016

`Why was the if-block above moved into the for-loop?

@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr Feb 9, 2016

Member

@mikadou Collcab rate is a wannabe optimization to reduce amount of collision test per triangle. It might have been broken from the start.

I'll gather all info I got from Ulteq until now and start a forum thread. We need to write an overview wikipage/doxypage about this.

EDIT: @mikadou @ulteq http://www.rigsofrods.com/threads/120897-Devs-only-Overview-of-inter-intra-truck-collision-system

Member

only-a-ptr commented Feb 9, 2016

@mikadou Collcab rate is a wannabe optimization to reduce amount of collision test per triangle. It might have been broken from the start.

I'll gather all info I got from Ulteq until now and start a forum thread. We need to write an overview wikipage/doxypage about this.

EDIT: @mikadou @ulteq http://www.rigsofrods.com/threads/120897-Devs-only-Overview-of-inter-intra-truck-collision-system

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq Feb 9, 2016

Contributor

Are cabs triangles/faces of the submesh?

Yes, cabs are triangles of the submesh and contacters are points.

What is collcabrate?

The collcabrate specifies how many physics cycles are to be skipped before the next collision check of a given cab is performed. So far, the rate just depends on how long ago the last collision happened.

How are rate and distance in the collcabrate_t structure related?

rate counts down, distance counts up.

rate: remaining amount of physics cycles to be skipped
distance: distance (in physics cycles) to the previous collision check

Why was the if-block above moved into the for-loop?

Inverting the transformation matrix is one of the most expensive calculations that needs to be done for each cab.

intraPointCD->hit_count is usually really small (< 10).

hitnode->iswheel is true for all hit_counts in 2-5% of all intraPointCD->hit_count > 0 cases.

And doing 2% less matrix inversions saves more than the extra bool comparison in the for loop.

Contributor

ulteq commented Feb 9, 2016

Are cabs triangles/faces of the submesh?

Yes, cabs are triangles of the submesh and contacters are points.

What is collcabrate?

The collcabrate specifies how many physics cycles are to be skipped before the next collision check of a given cab is performed. So far, the rate just depends on how long ago the last collision happened.

How are rate and distance in the collcabrate_t structure related?

rate counts down, distance counts up.

rate: remaining amount of physics cycles to be skipped
distance: distance (in physics cycles) to the previous collision check

Why was the if-block above moved into the for-loop?

Inverting the transformation matrix is one of the most expensive calculations that needs to be done for each cab.

intraPointCD->hit_count is usually really small (< 10).

hitnode->iswheel is true for all hit_counts in 2-5% of all intraPointCD->hit_count > 0 cases.

And doing 2% less matrix inversions saves more than the extra bool comparison in the for loop.

@mikadou

This comment has been minimized.

Show comment
Hide comment
@mikadou

mikadou Feb 9, 2016

Contributor

The collcabrate specifies how many physics cycles are to be skipped before the next collision check of a given cab is performed. So far, the rate just depends on how long ago the last collision happened.

rate: remaining amount of physics cycles to be skipped
distance: distance (in physics cycles) to the previous collision check

Thank you, this clarifies the intention 😸 Could you add these explanations as comments in the code?

Contributor

mikadou commented Feb 9, 2016

The collcabrate specifies how many physics cycles are to be skipped before the next collision check of a given cab is performed. So far, the rate just depends on how long ago the last collision happened.

rate: remaining amount of physics cycles to be skipped
distance: distance (in physics cycles) to the previous collision check

Thank you, this clarifies the intention 😸 Could you add these explanations as comments in the code?

@mikadou

This comment has been minimized.

Show comment
Hide comment
@mikadou

mikadou Feb 9, 2016

Contributor

Inverting the transformation matrix is one of the most expensive calculations that needs to be done for each cab.

intraPointCD->hit_count is usually really small (< 10).

hitnode->iswheel is true for all hit_counts in 2-5% of all intraPointCD->hit_count > 0 cases.

And doing 2% less matrix inversions saves more than the extra bool comparison in the for loop.

Makes sense, but the code becomes harder to understand. It might be a good idea to use a wrapper class around Matrix3 which performs lazy initialization.

I think it is even possible to avoid the matrix inversion completely:

bx = na->RelPosition - no->RelPosition;
by = nb->RelPosition - no->RelPosition;
bz = fast_normalise(bx.crossProduct(by));
//coordinates change matrix
Matrix3 forward( bx[0], bx[1], bx[2],
                           by[0], by[1]. by[2],
                           bz[0], bz[1], bz[2]);
Contributor

mikadou commented Feb 9, 2016

Inverting the transformation matrix is one of the most expensive calculations that needs to be done for each cab.

intraPointCD->hit_count is usually really small (< 10).

hitnode->iswheel is true for all hit_counts in 2-5% of all intraPointCD->hit_count > 0 cases.

And doing 2% less matrix inversions saves more than the extra bool comparison in the for loop.

Makes sense, but the code becomes harder to understand. It might be a good idea to use a wrapper class around Matrix3 which performs lazy initialization.

I think it is even possible to avoid the matrix inversion completely:

bx = na->RelPosition - no->RelPosition;
by = nb->RelPosition - no->RelPosition;
bz = fast_normalise(bx.crossProduct(by));
//coordinates change matrix
Matrix3 forward( bx[0], bx[1], bx[2],
                           by[0], by[1]. by[2],
                           bz[0], bz[1], bz[2]);
@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq Feb 9, 2016

Contributor

I think it is even possible to avoid the matrix inversion completely

The matrix inversion is where all the magic happens, we can't avoid it.

Matrix3 forward( bx[0], bx[1], bx[2],
                 by[0], by[1]. by[2],
                 bz[0], bz[1], bz[2]);

That would be the transpose of the matrix, but we need the inverse.

Contributor

ulteq commented Feb 9, 2016

I think it is even possible to avoid the matrix inversion completely

The matrix inversion is where all the magic happens, we can't avoid it.

Matrix3 forward( bx[0], bx[1], bx[2],
                 by[0], by[1]. by[2],
                 bz[0], bz[1], bz[2]);

That would be the transpose of the matrix, but we need the inverse.

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq Feb 9, 2016

Contributor

But changing:

forward.FromAxes(bx,by,bz);

to

forward = Matrix3(bx[0], by[0], bz[0],
                  bx[1], by[1]. bz[1],
                  bx[2], by[2], bz[2]);

might indeed speed things up a bit.

Edit: Just tested it. If anything, it's a little slower.

Contributor

ulteq commented Feb 9, 2016

But changing:

forward.FromAxes(bx,by,bz);

to

forward = Matrix3(bx[0], by[0], bz[0],
                  bx[1], by[1]. bz[1],
                  bx[2], by[2], bz[2]);

might indeed speed things up a bit.

Edit: Just tested it. If anything, it's a little slower.

@mikadou

This comment has been minimized.

Show comment
Hide comment
@mikadou

mikadou Feb 9, 2016

Contributor

The matrix inversion is where all the magic happens, we can't avoid it.

Hmm, here's my though process:

  • We have a triangle defined by three points n0, na, nb
  • We have two vectors bx, by spanning the plane defined by the collision triangle
  • bz is the normal vector of this plane
  • We have the coordinates of point p relative to n0 (in world coordinates)
  • Now we project the point p onto the three vectors bx, by, bz
    • i.e. p_projected_onto_bx = bx[0] * p[0] + bx[1] * p[1] + bx[2] * p[2]

What am I missing?

Contributor

mikadou commented Feb 9, 2016

The matrix inversion is where all the magic happens, we can't avoid it.

Hmm, here's my though process:

  • We have a triangle defined by three points n0, na, nb
  • We have two vectors bx, by spanning the plane defined by the collision triangle
  • bz is the normal vector of this plane
  • We have the coordinates of point p relative to n0 (in world coordinates)
  • Now we project the point p onto the three vectors bx, by, bz
    • i.e. p_projected_onto_bx = bx[0] * p[0] + bx[1] * p[1] + bx[2] * p[2]

What am I missing?

@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr Feb 9, 2016

Member

@mikadou It's quite possible there's a solution which doesn't require the Matrix::inverse() call but computes things directly. I dunno about Ulteq, but personally I can't do math. I know how to use matrices for coordinate transforms, but I can only construct them usin OGRE's math API.

Your solution sounds somewhat familiar. Can you explain it more? I assume p_projected_onto_bx is a scalar, but what is it's meaning? Also, I assume bx[0] means "X component of bx vector", or bx.x. Write a clearer example, please.

Member

only-a-ptr commented Feb 9, 2016

@mikadou It's quite possible there's a solution which doesn't require the Matrix::inverse() call but computes things directly. I dunno about Ulteq, but personally I can't do math. I know how to use matrices for coordinate transforms, but I can only construct them usin OGRE's math API.

Your solution sounds somewhat familiar. Can you explain it more? I assume p_projected_onto_bx is a scalar, but what is it's meaning? Also, I assume bx[0] means "X component of bx vector", or bx.x. Write a clearer example, please.

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq Feb 9, 2016

Contributor

What am I missing?

p_projected_onto_bx will only give us the distance from the triangle.

But we still need to check if the point is inside of the triangle or not.

Edit: Aside from that, we still need the point in triangle local coordinates later on:

a)

//Find the point's velocity relative to the triangle
vecrelVel = (hitnode->Velocity - (no->Velocity * (-point.x - point.y + 1.0f) + na->Velocity * point.x + nb->Velocity * point.y));

b)

//The force that the triangle puts on the point
float trfnormal = (no->Forces * (-point.x - point.y + 1.0f) + na->Forces * point.x
                    + nb->Forces * point.y).dotProduct(plnormal);

c)

no->Forces -= (-point.x - point.y + 1.0f) * forcevec;
na->Forces -= (point.x) * forcevec;
nb->Forces -= (point.y) * forcevec;
Contributor

ulteq commented Feb 9, 2016

What am I missing?

p_projected_onto_bx will only give us the distance from the triangle.

But we still need to check if the point is inside of the triangle or not.

Edit: Aside from that, we still need the point in triangle local coordinates later on:

a)

//Find the point's velocity relative to the triangle
vecrelVel = (hitnode->Velocity - (no->Velocity * (-point.x - point.y + 1.0f) + na->Velocity * point.x + nb->Velocity * point.y));

b)

//The force that the triangle puts on the point
float trfnormal = (no->Forces * (-point.x - point.y + 1.0f) + na->Forces * point.x
                    + nb->Forces * point.y).dotProduct(plnormal);

c)

no->Forces -= (-point.x - point.y + 1.0f) * forcevec;
na->Forces -= (point.x) * forcevec;
nb->Forces -= (point.y) * forcevec;
@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq Feb 9, 2016

Contributor

Things we could try to speed it up:

  • Cache the transformation matrix and reuse it if the cab did not move
  • Quit early if the distance between the point and the triangle is greater than the collrange

Edit1: Just tested it. Both ideas actually make it slower.

Edit2: I think the best solution would be to have a collcabrate that changes with velocity.

Contributor

ulteq commented Feb 9, 2016

Things we could try to speed it up:

  • Cache the transformation matrix and reuse it if the cab did not move
  • Quit early if the distance between the point and the triangle is greater than the collrange

Edit1: Just tested it. Both ideas actually make it slower.

Edit2: I think the best solution would be to have a collcabrate that changes with velocity.

@mikadou

This comment has been minimized.

Show comment
Hide comment
@mikadou

mikadou Feb 9, 2016

Contributor

@only-a-ptr apparently I can't do math either. I'm afraid @ulteq is correct that there is no more efficient way.

Contributor

mikadou commented Feb 9, 2016

@only-a-ptr apparently I can't do math either. I'm afraid @ulteq is correct that there is no more efficient way.

ulteq added a commit that referenced this pull request Feb 9, 2016

Merge pull request #757 from ulteq/collcabrate-revert
[Cleanup] Cleaned up the collcabrate calculation

@ulteq ulteq merged commit c5da330 into RigsOfRods:master Feb 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ulteq ulteq deleted the ulteq:collcabrate-revert branch Feb 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment