Re: Collider and Mover code

[Gervase Lam]

> Date: Sat, 25 Jan 2003 11:26:25 +0100
> From: David Philippi <address@hidden>
> Subject: Re: Collider and Mover code

> Let's start with the suggestions/corrections:
>
> - templates parameters should be named by using typename instead of
> class= ,=20
> there are cases where this is much easier to read

This is an oversight due to the fact that PinguCollider changed to 
Collider while the template typename parameter was still left as Collider. 
 I've changed it from Collider to ColliderFunctor.

> mover.hxx:
> - the template requires an #include vector.hxx since you actually use
> one

I'll do the changes.

> collider.hxx:
> - this one doesn't requires an #include vector.hxx, it may be moved
> to=20 collider.cxx as well as the world.hxx

I'll do the changes.

> upright_pingu_collider.cxx
> - either use bool collide or return true/false - right now the bool
> is=20 useless. Using a compiler with named return value optimization
> collided=3D= true;=20
> break; and a return collided at the end or using direct returns should
> cr= eate=20
> identical code

bool collide is a class member that is needed by collided().  I intend to 
use collided() in the faller code.  Therefore, I've changed the code so 
that it uses a break.

> - the static_cast<float>(pingu_height) is redundant - pos.y is a float
> so= the=20
> result type of pos.y - pingu_height is already float. You only need to
> ca= st=20
> if there's no float involved otherwise

I've made the change.  However, it would be "interesting" if pingu_height 
were to change to a double in the far future, for example.  But I think 
the likelihood of that happening is practically zero.

> linear_mover.hxx
> - why are the Collider parameters passed by value instead of const
> refere= nce?

Collider is a Function Object.  So what is passed in is usually a 
temporary object.  A quote from my STL book on Function Objects:

"Passing function objects by value instead of by reference has the 
advantage that you can pass constant and temporary expressions."

If you want references to be used, you can with function objects.  In STL, 
you can do the following:

// Functor that gives a sequence of nos. starting with 1 and increments
// each time it is called.
IntSeq seq(1);
list<int> coll;

// Generate a list of numbers 1, 2, 3, 4, 5.
generate_n<back_insert_iterator<list<int> >, int, IntSeq&>
  (back_inserter(coll), 5, seq);

I couldn't think of a simpler general example, so I adapted one from my 
STL book.  This needs to be done using references so that seq keeps its 
value.

Also note that seq is not const because it will internally have the values 
1, 2, 3, 4 and 5 while generate_n() is being run.  Using non-const 
references also allows function objects to handle situations that require 
feedback, with the feedback information being stored in the function 
object.

For Colliders and Movers, the example is simpler:

// Set up Colliders
falling_down_pingu UprightPinguCollider(true);
falling_up_pingu UprightPinguCollider(false);

// Instantiate a Linear Mover
LinearMover<UprightPinguCollider&> mover(world, init_pos);

// Update the position of the Pingu
mover.update(move_vector, falling_down_pingu);
mover.update(move_vector, falling_up_pingu);

In this case, mover is taking in falling_down_pingu and falling_up_pingu 
by reference.  This means that in the future, (non-const) colliders could 
receive feedback information.

Letting the person using linear_mover decide what the type of the 
parameter is going to be provides more flexibility.  The person is not 
forced make a value be passable by reference.  If required, the person can 
use a temporary.

I did think about this while I was coding it.  In STL, classically 
temporaries are used.  This would mean that there would be the 
instantiation and destruction of the temporary object.  However, because 
that temporary object is passed to the function that needs a function 
object, there would also be the instantiation and destruction of the 
object within this function.  In other words, two objects are being 
created and destroyed.

In the end, I decided that it would be clearer to do it the classical way. 
 I think people can see this more easily.  It is also flexible in that the 
person can ask for the Function Object to be passed by reference if 
needed, or not.  The person has a choice.

Also, Ingo gave me a couple of possible examples of using Function Objects:

LinearTerrainTraverser traverser (colmap, pos, velocity, 
CollisionFunctor());

GroundTerrainTraverser traverser (colmap, pos, velocity, Direction::LEFT, 
CollisionFunctor());

Therefore, I don't know how rigidly I should stick to the use of temporary 
objects or whether I should pass a reference as a parameter to the 
Collider template.

> - why does the for loop in update use a float as counter?

I've changed the counter i to an int.  I assume that the expression (i < 
move_length) would promote i to a float before doing the comparison.

In this case, I would be extremely tempted to do (static_cast<float>(i) < 
move_length) as the type of move_length is not within a few lines of the 
expression.  Therefore, people could accidentally think that move_length 
is an int.  I think it is better to be explicit rather than implicit.  
Then things are immediately obvious.

I'll wait for the answers before posting the updates for this.  I also 
need to sort out a strange compilation problem.

I did some of the above changes and I got the following compilation 
messages:

actions/bomber.cxx|96| undefined reference to 
`Colliders::UprightPinguCollider::UprightPinguCollider(bool)'
actions/bomber.cxx|98| undefined reference to 
`Colliders::UprightPinguCollider::UprightPinguCollider(bool)'

I then reverted back to the code I had without any of the above changes 
and I still get the same messages!  I even checked with 'find . -name "*" 
| xargs ls -altrd' to check that I had backed out the files that I had 
changed.

I even used 'nm -s -l src/colliders/libpingus_colliders.a' to check that 
the UprightPinguCollider constructor existed there.  It does!  Very odd.  
At the moment, I can't think what else to check.

Thanks,
Gervase.