Adding car physics racing demo

Adding car physics racing demo

[dominich, 2012-07-24 22:39:30]

What do you think?

[rupertk, 2012-07-25 20:04:59]

Awesome demo!  From a concept review point of view, I patched it in and (after
some monkeying -- see racer.html comment) it ran really well.  That might be
enough review!

However, given you invited, I've done a /reasonably/ full job on it.  Man, I'm
going to regret this level of verbosity/pedantry on my next code reivew ;-)

HTH, R.

http://codereview.appspot.com/6440043/diff/1/demos/demo.dart
File demos/demo.dart (right):

http://codereview.appspot.com/6440043/diff/1/demos/demo.dart#newcode65
demos/demo.dart:65: }
This should use a "Redirecting Constructors" (spec. term).
Something like...
Demo([num viewportScale = _VIEWPORT_SCALE]) : this.withGravity(new Vector(0,
GRAVITY), viewportScale);

http://codereview.appspot.com/6440043/diff/1/demos/demo.dart#newcode78
demos/demo.dart:78: ctx.fillRect(0, 0, CANVAS_WIDTH, CANVAS_HEIGHT);
Curious why chose to change colour.  Sure it's fine but I /suspect/
setFillColor() + fillRect() will be slower than setting the colour in CSS and
keeping clearRect().  (One JS Canvas performance tip is to reduce state changes
such as colour changes.)
Hope demos can show best practise (if true).

http://codereview.appspot.com/6440043/diff/1/demos/racer/Car.dart
File demos/racer/Car.dart (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/Car.dart#newcode46
demos/racer/Car.dart:46: jointDef.bodyB = blTire._body;
I know, private is library-private, not class-private but...

http://codereview.appspot.com/6440043/diff/1/demos/racer/Car.dart#newcode49
demos/racer/Car.dart:49: _tires[0] = blTire;
This tire creation code is v. repetitititive ;-)

http://codereview.appspot.com/6440043/diff/1/demos/racer/Car.dart#newcode77
demos/racer/Car.dart:77: for (var i = 0; i < _tires.length; ++i)
Given this is called every frame and is always 4, extract to constant here and
in initializer? (and perhaps reverse loop order (i = NUM_TIRES - 1; i >= 0;
i--))

http://codereview.appspot.com/6440043/diff/1/demos/racer/Car.dart#newcode80
demos/racer/Car.dart:80: _tires[i].updateDrive(controlState);
Curly braces?

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart
File demos/racer/Tire.dart (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode7
demos/racer/Tire.dart:7: double _currentTraction;
It would be nice to see all of these as runtime constants (e.g. final,
constructed in factory).

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode33
demos/racer/Tire.dart:33: }
Contance would remove need for this which don't believe used otherwise.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode43
demos/racer/Tire.dart:43: var index = _groundAreas.indexOf(ga);
var better as final here (to protect accidental assignment).

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode61
demos/racer/Tire.dart:61: Vector get lateralVelocity() {
Can be private.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode62
demos/racer/Tire.dart:62: Vector currentRightNormal = _body.getWorldVector(new
Vector(1.0, 0.0));
lateralVelocity(), forwardVelocity() and updateFriction() are creating the
Vectors each frame.  In performant production, I'd measure the effect of this
and likely (depending on GC-code-maturity) switch them to register-like members.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode67
demos/racer/Tire.dart:67: Vector get forwardVelocity() {
Can be private.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode76
demos/racer/Tire.dart:76: impulse.mulLocal(_maxLateralImpulse / impulse.length);
eh-hum, curly braces.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode100
demos/racer/Tire.dart:100: double currentSpeed = Vector.dot(forwardVelocity,
currentForwardNormal);
Something's bugging me about this (and duplication in "get forwardVelocity()")
but my brain's about done for the day.  It's probably fine but...

I /think/ it could be factored-out to "get forwardSpeed()" (what I think this is
doing) when we'd find simply taking the length of the forwardVelocity() might be
sufficient.

One for part 2?

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode105
demos/racer/Tire.dart:105: force = _maxDriveForce;
Curly braces.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart
File demos/racer/racer.dart (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode1
demos/racer/racer.dart:1: #library('racer');
Raises an interesting point -- filename capitalization.
Dart Style Guide says "DO name libraries and sourcefiles using
lowercase_with_underscores".  I guess this is lower-case because of this? 
Shouldn't all [new] source files therefore also be lower-case?
(Personally, I prefer UpperCamelCase for all but I do try to follow and do care
about consistency.)

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode28
demos/racer/racer.dart:28: {
Yuck!  Can we separate these as well named functions thereby removing need for
comments (see BoxTest.dart)

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode42
demos/racer/racer.dart:42: groundAreaFixture.userData = new GroundArea(0.001,
false);
Would be better done using userData on FixtureDef (no need of
groundAreaFixture).  I.e:


      fixtureDef.userData = new GroundArea(0.001, false);
      _groundBody.createFixture(fixtureDef);

      shape.setAsBoxWithCenterAndAngle(
          27, 15, new Vector(20, 40), MathBox.degToRad(-40));
      fixtureDef.userData = new GroundArea(0.2, false);
      _groundBody.createFixture(fixtureDef);

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode62
demos/racer/racer.dart:62: final int boundaryY = 100;
Always happy to see "final" but personally curious why not everywhere? 
(personal edification)

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode122
demos/racer/racer.dart:122: _tireVsGroundArea(fudB, fudA, began);
Curly-braces?

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode137
demos/racer/racer.dart:137: tire.removeGroundArea(groundArea);
Curly-braces?

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode148
demos/racer/racer.dart:148: ctx.fillText('Use the arrow keys to control the
car', 20, 580);
While this is consistent with rest of demos, given this particular demo will
likely be used as basis for future games, I wonder if we should do the
performant thing here?  (not re-render text each frame)

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode153
demos/racer/racer.dart:153: Car _car;
Consistency: I quite like having members at the bottom but most of this library
puts at top.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart.js.map
File demos/racer/racer.dart.js.map (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart.js.map#ne...
demos/racer/racer.dart.js.map:6: }
.js.map file included but not .js file?

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.html
File demos/racer/racer.html (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.html#newcode1
demos/racer/racer.html:1: <!DOCTYPE html>
I note you decided not to add Racer to demos.html.  Intentional?  (better
encapsulation of demo that uses multiple dart files?)  Perhaps adding a link
from demos.html would be handy for first-timers?
Either way, we'll need to update the build.xml -- probably more so if we were to
put all racer files in the demos directory!

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.html#newcode4
demos/racer/racer.html:4: <title>Racer</title>
Rietveld thinks you're modifying benchmarks.html.  Might be OK in your workspace
but probably want to double-check before push.

Details:
The following might be a bug in Rietveld / upload.py? :

Both Rietveld and downloading the patches and applying them all modifies
benchmarks.html to use racer.dart (rather than creating demos/racer/racer.html. 
This seems caused by the header in *7002* reading: diff --git a/benchmarks.html
b/demos/racer/racer.html
Odd.  Modified manually to test.

(p.s. Shame git doesn't provide a way to push whole batch of changes as it
stands in one's workspace.  Oh, hang on ;-) )

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.html#newcode7
demos/racer/racer.html:7: maximum-scale=1.0, user-scalable=0">
Other demos don't suffer (no arrow keys) but if browser is smaller than canvas,
scroll activates on arrow-keys.  Consider adding style="overflow-x: hidden" per
http://stackoverflow.com/questions/6111823/disable-horizontal-scrollbar-due-t...

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.html#newcode14
demos/racer/racer.html:14: <script type='text/javascript'
For my own edification: why change double to single quotes?

http://codereview.appspot.com/6440043/diff/1/lib/collision/shapes/PolygonShap...
File lib/collision/shapes/PolygonShape.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/collision/shapes/PolygonShap...
lib/collision/shapes/PolygonShape.dart:46: normals = new
List<Vector>(Settings.MAX_POLYGON_VERTICES),
This code seems inefficient to me unless one is often going to be changing the
number of verticies.  If you agree, perhaps we should bug switching it to using
a variable-length list?
(know this isn't what you're changing here but since you drew the eye with the
TODO ;-) )

http://codereview.appspot.com/6440043/diff/1/lib/collision/shapes/PolygonShap...
lib/collision/shapes/PolygonShape.dart:97: * TODO(dominich): Remove [count].
Guess implies we no longer want to be able to copy a subset of supplied
'otherVertices' (which is what I guess this is for).  Correct deduction?
Fair enough -- seems exceedingly uncommon.

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart
File lib/common/MathBox.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart#newcode20
lib/common/MathBox.dart:20: MathBox() { }
Think we need a bug to fix this static-methods class!
(doesn't need constructor and probably ought to be functions in non-class file
that one pulls-in with namespace.)

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart#newcode38
lib/common/MathBox.dart:38: /** Returns the closest value to 'a' that is in
between 'low' and 'high' */
Fancy fixing these 'a' to [a], etc while in here?

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart#newcode57
lib/common/MathBox.dart:57: static num radToDeg(num rad) => rad * (180.0 /
Math.PI);
1. Performance: Better to place numeric constants first since dart2js optimizes
better.

2. Question: I know dart2js will turn these into compile-time constants.  I
assume Dartium effectively does similarly (load-time)?

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/Body.dart
File lib/dynamics/Body.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/Body.dart#newcode94
lib/dynamics/Body.dart:94: : flags = 0,
I know this conforms with the style guide but I find it uuugly ;-)

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/World.dart
File lib/dynamics/World.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/World.dart#newcode1139
lib/dynamics/World.dart:1139: _debugDraw.drawPolygon(vertices, vertexCount,
color);
Missing curly-braces in this if-else.

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/World.dart#newcode1140
lib/dynamics/World.dart:1140: }
I'm sure you're right but curious what the problem was? (couldn't spot
difference in cardemo.)

[dominich, 2012-07-25 21:50:18]

Have at it.

http://codereview.appspot.com/6440043/diff/1/demos/demo.dart
File demos/demo.dart (right):

http://codereview.appspot.com/6440043/diff/1/demos/demo.dart#newcode65
demos/demo.dart:65: }
On 2012/07/25 20:04:59, rupertk wrote:
> This should use a "Redirecting Constructors" (spec. term).
> Something like...
> Demo([num viewportScale = _VIEWPORT_SCALE]) : this.withGravity(new Vector(0,
> GRAVITY), viewportScale);

Done.

http://codereview.appspot.com/6440043/diff/1/demos/demo.dart#newcode78
demos/demo.dart:78: ctx.fillRect(0, 0, CANVAS_WIDTH, CANVAS_HEIGHT);
On 2012/07/25 20:04:59, rupertk wrote:
> Curious why chose to change colour.  Sure it's fine but I /suspect/
> setFillColor() + fillRect() will be slower than setting the colour in CSS and
> keeping clearRect().  (One JS Canvas performance tip is to reduce state
changes
> such as colour changes.)
> Hope demos can show best practise (if true).

Good point - also moved the text into HTML instead of drawing it on canvas.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Car.dart
File demos/racer/Car.dart (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/Car.dart#newcode46
demos/racer/Car.dart:46: jointDef.bodyB = blTire._body;
On 2012/07/25 20:04:59, rupertk wrote:
> I know, private is library-private, not class-private but... 

And I have a problem with this aspect of the language. It means I either want to
make every field private (because I will expose public getters where necessary)
or nothing private (because it looks weird to have classes accessing privates
with the same library).

http://codereview.appspot.com/6440043/diff/1/demos/racer/Car.dart#newcode49
demos/racer/Car.dart:49: _tires[0] = blTire;
On 2012/07/25 20:04:59, rupertk wrote:
> This tire creation code is v. repetitititive ;-)

I'm open to feedback on how to improve it. Next patch is a little better.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Car.dart#newcode80
demos/racer/Car.dart:80: _tires[i].updateDrive(controlState);
On 2012/07/25 20:04:59, rupertk wrote:
> Curly braces?

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart
File demos/racer/Tire.dart (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode7
demos/racer/Tire.dart:7: double _currentTraction;
On 2012/07/25 20:04:59, rupertk wrote:
> It would be nice to see all of these as runtime constants (e.g. final,
> constructed in factory).

Done. final, anyway.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode33
demos/racer/Tire.dart:33: }
On 2012/07/25 20:04:59, rupertk wrote:
> Contance would remove need for this which don't believe used otherwise.

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode43
demos/racer/Tire.dart:43: var index = _groundAreas.indexOf(ga);
On 2012/07/25 20:04:59, rupertk wrote:
> var better as final here (to protect accidental assignment).

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode61
demos/racer/Tire.dart:61: Vector get lateralVelocity() {
On 2012/07/25 20:04:59, rupertk wrote:
> Can be private.

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode62
demos/racer/Tire.dart:62: Vector currentRightNormal = _body.getWorldVector(new
Vector(1.0, 0.0));
On 2012/07/25 20:04:59, rupertk wrote:
> lateralVelocity(), forwardVelocity() and updateFriction() are creating the
> Vectors each frame.  In performant production, I'd measure the effect of this
> and likely (depending on GC-code-maturity) switch them to register-like
members.

This is one other reason to switch to DartVectorMath - it tries very hard not to
create vectors/matrices.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode67
demos/racer/Tire.dart:67: Vector get forwardVelocity() {
On 2012/07/25 20:04:59, rupertk wrote:
> Can be private.

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode76
demos/racer/Tire.dart:76: impulse.mulLocal(_maxLateralImpulse / impulse.length);
On 2012/07/25 20:04:59, rupertk wrote:
> eh-hum, curly braces.

So much wasted vertical space. ;)

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode100
demos/racer/Tire.dart:100: double currentSpeed = Vector.dot(forwardVelocity,
currentForwardNormal);
On 2012/07/25 20:04:59, rupertk wrote:
> Something's bugging me about this (and duplication in "get forwardVelocity()")
> but my brain's about done for the day.  It's probably fine but...
> 
> I /think/ it could be factored-out to "get forwardSpeed()" (what I think this
is
> doing) when we'd find simply taking the length of the forwardVelocity() might
be
> sufficient.
> 
> One for part 2?

I had this using the forwardVelocity length but then I need the normal anyway
below when applying the force.

http://codereview.appspot.com/6440043/diff/1/demos/racer/Tire.dart#newcode105
demos/racer/Tire.dart:105: force = _maxDriveForce;
On 2012/07/25 20:04:59, rupertk wrote:
> Curly braces.

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart
File demos/racer/racer.dart (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode1
demos/racer/racer.dart:1: #library('racer');
On 2012/07/25 20:04:59, rupertk wrote:
> Raises an interesting point -- filename capitalization.
> Dart Style Guide says "DO name libraries and sourcefiles using
> lowercase_with_underscores".  I guess this is lower-case because of this? 
> Shouldn't all [new] source files therefore also be lower-case?
> (Personally, I prefer UpperCamelCase for all but I do try to follow and do
care
> about consistency.)

I prefer lower-case for files and will rename accordingly.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode28
demos/racer/racer.dart:28: {
On 2012/07/25 20:04:59, rupertk wrote:
> Yuck!  Can we separate these as well named functions thereby removing need for
> comments (see BoxTest.dart)

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode42
demos/racer/racer.dart:42: groundAreaFixture.userData = new GroundArea(0.001,
false);
On 2012/07/25 20:04:59, rupertk wrote:
> Would be better done using userData on FixtureDef (no need of
> groundAreaFixture).  I.e:
> 
> 
>       fixtureDef.userData = new GroundArea(0.001, false);
>       _groundBody.createFixture(fixtureDef);
> 
>       shape.setAsBoxWithCenterAndAngle(
>           27, 15, new Vector(20, 40), MathBox.degToRad(-40));
>       fixtureDef.userData = new GroundArea(0.2, false);
>       _groundBody.createFixture(fixtureDef);

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode62
demos/racer/racer.dart:62: final int boundaryY = 100;
On 2012/07/25 20:04:59, rupertk wrote:
> Always happy to see "final" but personally curious why not everywhere? 
> (personal edification)

Lack of rigour on my part.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode122
demos/racer/racer.dart:122: _tireVsGroundArea(fudB, fudA, began);
On 2012/07/25 20:04:59, rupertk wrote:
> Curly-braces?

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode137
demos/racer/racer.dart:137: tire.removeGroundArea(groundArea);
On 2012/07/25 20:04:59, rupertk wrote:
> Curly-braces?

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode148
demos/racer/racer.dart:148: ctx.fillText('Use the arrow keys to control the
car', 20, 580);
On 2012/07/25 20:04:59, rupertk wrote:
> While this is consistent with rest of demos, given this particular demo will
> likely be used as basis for future games, I wonder if we should do the
> performant thing here?  (not re-render text each frame)

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode153
demos/racer/racer.dart:153: Car _car;
On 2012/07/25 20:04:59, rupertk wrote:
> Consistency: I quite like having members at the bottom but most of this
library
> puts at top.

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.html
File demos/racer/racer.html (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.html#newcode1
demos/racer/racer.html:1: <!DOCTYPE html>
On 2012/07/25 20:04:59, rupertk wrote:
> I note you decided not to add Racer to demos.html.  Intentional?  (better
> encapsulation of demo that uses multiple dart files?)  Perhaps adding a link
> from demos.html would be handy for first-timers?
> Either way, we'll need to update the build.xml -- probably more so if we were
to
> put all racer files in the demos directory!

Updating build.xml is on my todo list now it's checked in :)

Not intentional - I'll add it.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.html#newcode7
demos/racer/racer.html:7: maximum-scale=1.0, user-scalable=0">
On 2012/07/25 20:04:59, rupertk wrote:
> Other demos don't suffer (no arrow keys) but if browser is smaller than
canvas,
> scroll activates on arrow-keys.  Consider adding style="overflow-x: hidden"
per
>
http://stackoverflow.com/questions/6111823/disable-horizontal-scrollbar-due-t...

Done.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.html#newcode14
demos/racer/racer.html:14: <script type='text/javascript'
On 2012/07/25 20:04:59, rupertk wrote:
> For my own edification: why change double to single quotes?

no good reason.

http://codereview.appspot.com/6440043/diff/1/lib/collision/shapes/PolygonShap...
File lib/collision/shapes/PolygonShape.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/collision/shapes/PolygonShap...
lib/collision/shapes/PolygonShape.dart:46: normals = new
List<Vector>(Settings.MAX_POLYGON_VERTICES),
On 2012/07/25 20:04:59, rupertk wrote:
> This code seems inefficient to me unless one is often going to be changing the
> number of verticies.  If you agree, perhaps we should bug switching it to
using
> a variable-length list?
> (know this isn't what you're changing here but since you drew the eye with the
> TODO ;-) )

I wonder if there's a performance issue if it's variable even if we don't call
add. Ie, there might be assumptions when it's fixed-length that kick in in the
VM.

worth bugging anyway.

http://codereview.appspot.com/6440043/diff/1/lib/collision/shapes/PolygonShap...
lib/collision/shapes/PolygonShape.dart:97: * TODO(dominich): Remove [count].
On 2012/07/25 20:04:59, rupertk wrote:
> Guess implies we no longer want to be able to copy a subset of supplied
> 'otherVertices' (which is what I guess this is for).  Correct deduction?
> Fair enough -- seems exceedingly uncommon.

Good point - at first glance i thought it was just redundancy given the list has
the length already. but this is something that people might be using... though
without a start offset it's questionably useful.

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart
File lib/common/MathBox.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart#newcode20
lib/common/MathBox.dart:20: MathBox() { }
On 2012/07/25 20:04:59, rupertk wrote:
> Think we need a bug to fix this static-methods class!
> (doesn't need constructor and probably ought to be functions in non-class file
> that one pulls-in with namespace.)

that and get clamp and degToRad and radToDeg added to Dart core.

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart#newcode38
lib/common/MathBox.dart:38: /** Returns the closest value to 'a' that is in
between 'low' and 'high' */
On 2012/07/25 20:04:59, rupertk wrote:
> Fancy fixing these 'a' to [a], etc while in here?

Done.

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart#newcode57
lib/common/MathBox.dart:57: static num radToDeg(num rad) => rad * (180.0 /
Math.PI);
On 2012/07/25 20:04:59, rupertk wrote:
> 1. Performance: Better to place numeric constants first since dart2js
optimizes
> better.
> 
> 2. Question: I know dart2js will turn these into compile-time constants.  I
> assume Dartium effectively does similarly (load-time)?

I assume the same.

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/World.dart
File lib/dynamics/World.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/World.dart#newcode1139
lib/dynamics/World.dart:1139: _debugDraw.drawPolygon(vertices, vertexCount,
color);
On 2012/07/25 20:04:59, rupertk wrote:
> Missing curly-braces in this if-else.

Done.

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/World.dart#newcode1140
lib/dynamics/World.dart:1140: }
On 2012/07/25 20:04:59, rupertk wrote:
> I'm sure you're right but curious what the problem was? (couldn't spot
> difference in cardemo.)

Trying to draw the boundary, which is an 'edge', failed as fill doesn't fill
shapes without an interior. We could change this to stroke and fill solid
polygons, and that would work too, but this seemed more generally correct.

[rupertk, 2012-07-26 16:36:56]

Round 2, ding ding!

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart
File demos/racer/racer.dart (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode153
demos/racer/racer.dart:153: Car _car;
On 2012/07/25 21:50:18, dominich wrote:
> On 2012/07/25 20:04:59, rupertk wrote:
> > Consistency: I quite like having members at the bottom but most of this
> library
> > puts at top.
> 
> Done.

Done?  (meaning decided against consistency with library but consistent with
other racer files?)

http://codereview.appspot.com/6440043/diff/1/lib/collision/shapes/PolygonShap...
File lib/collision/shapes/PolygonShape.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/collision/shapes/PolygonShap...
lib/collision/shapes/PolygonShape.dart:46: normals = new
List<Vector>(Settings.MAX_POLYGON_VERTICES),
On 2012/07/25 21:50:18, dominich wrote:
> On 2012/07/25 20:04:59, rupertk wrote:
> > This code seems inefficient to me unless one is often going to be changing
the
> > number of verticies.  If you agree, perhaps we should bug switching it to
> using
> > a variable-length list?
> > (know this isn't what you're changing here but since you drew the eye with
the
> > TODO ;-) )
> 
> I wonder if there's a performance issue if it's variable even if we don't call
> add. Ie, there might be assumptions when it's fixed-length that kick in in the
> VM.
> 
> worth bugging anyway.

k, logged as bug 14
https://code.google.com/p/dartbox2d/issues/detail?id=14

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart
File lib/common/MathBox.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart#newcode20
lib/common/MathBox.dart:20: MathBox() { }
On 2012/07/25 21:50:18, dominich wrote:
> On 2012/07/25 20:04:59, rupertk wrote:
> > Think we need a bug to fix this static-methods class!
> > (doesn't need constructor and probably ought to be functions in non-class
file
> > that one pulls-in with namespace.)
> 
> that and get clamp and degToRad and radToDeg added to Dart core.

Will leave bugging this to you (since you know how to phrase better.)

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/World.dart
File lib/dynamics/World.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/World.dart#newcode1140
lib/dynamics/World.dart:1140: }
On 2012/07/25 21:50:18, dominich wrote:
> On 2012/07/25 20:04:59, rupertk wrote:
> > I'm sure you're right but curious what the problem was? (couldn't spot
> > difference in cardemo.)
> 
> Trying to draw the boundary, which is an 'edge', failed as fill doesn't fill
> shapes without an interior. We could change this to stroke and fill solid
> polygons, and that would work too, but this seemed more generally correct.

hm, ok, will take your word for it.
Disabling this change seems to produce no discernible difference in racer on
mine.  It's only 1 extra comparison in debug-graphics :-)

http://codereview.appspot.com/6440043/diff/13005/demos.html
File demos.html (right):

http://codereview.appspot.com/6440043/diff/13005/demos.html#newcode62
demos.html:62: menu.appendChild(item);
Might it be neater to add 'racer/racer' to the 'demos' array? (seems to work)

http://codereview.appspot.com/6440043/diff/13005/demos/demo.dart
File demos/demo.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/demo.dart#newcode66
demos/demo.dart:66: : this.withGravity(new Vector(0, GRAVITY), viewportScale);
Seems fine.
I'm curious why you chose against optional parameter?  (my feeling is that it
ought to make for a more digestible API.  However this is only demo code so...)

http://codereview.appspot.com/6440043/diff/13005/demos/demo.dart#newcode81
demos/demo.dart:81: if (this.name != null) {
Shouldn't this be !==

http://codereview.appspot.com/6440043/diff/13005/demos/racer/car.dart
File demos/racer/car.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/racer/car.dart#newcode24
demos/racer/car.dart:24: final RevoluteJointDef jointDef = new
RevoluteJointDef();
Checking my understanding: if we had WeldJoint implemented, this could use that
instead of a RevoluteJoint, right?

It seems like it's merely a body at an angle (that is dynamically set) to
another body.  I guess with RJ and at the right scale (or desired simulation
accuracy), collisions might twist the wheel from its chosen angle?

http://codereview.appspot.com/6440043/diff/13005/demos/racer/car.dart#newcode53
demos/racer/car.dart:53: _frJoint = world.createJoint(jointDef);
Per "welcome suggestions on how to improve it", best I could come up with
(below) isn't great.  Kind'a needs multi-value returns but even then...

    _blTire = _createTire(world, jointDef, false, true);
    world.createJoint(jointDef);
    _brTire = _createTire(world, jointDef, false, false);
    world.createJoint(jointDef);

    _flTire = _createTire(world, jointDef, true, true);
    _flJoint = world.createJoint(jointDef);

    _frTire = _createTire(world, jointDef, true, false);
    _frJoint = world.createJoint(jointDef);
  }

  Tire _createTire(final World world, final RevoluteJointDef jointDef, final
bool front, final bool left) {
    final num x = left ? -3 : 3;
    num y, maxDriveForce, maxLateralImpulse;
    if (front) {
      y = 8.5;
      maxDriveForce = frontTireMaxDriveForce;
      maxLateralImpulse = frontTireMaxLateralImpulse;
    } else {
      y = 0.75;
      maxDriveForce = backTireMaxDriveForce;
      maxLateralImpulse = backTireMaxLateralImpulse;
    }
    final Tire tire = new Tire(world, maxForwardSpeed, maxBackwardSpeed,
        maxDriveForce, maxLateralImpulse);
    jointDef.bodyB = tire._body;
    jointDef.localAnchorA.setCoords(x, y);
    return tire;
  }

http://codereview.appspot.com/6440043/diff/13005/demos/racer/car.dart#newcode78
demos/racer/car.dart:78: final double turnPerTimeStep = turnSpeedPerSec / 60.0;
Actually, couldn't you just move all these out of the method now?

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.dart
File demos/racer/racer.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.dart#newco...
demos/racer/racer.dart:22: String get name() => null;
twitch, twitch

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.dart#newco...
demos/racer/racer.dart:78: _createBoundary();
Hm, why put these lower-level (more detailed) methods *above* the method that
calls them?
(I know JS-lint requires this but don't believe Dart does and I believe it's
more common to flow 'down' for readability.)
(perhaps for minimizing diff with previous?)

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.html
File demos/racer/racer.html (right):

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.html#newcode1
demos/racer/racer.html:1: <!DOCTYPE html>
Not really this file but...
How come you included the .js.map file but not the .js file?

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.html#newcode2
demos/racer/racer.html:2: <html style="overflow-x: hidden">
Sorry, realised it should have been "overflow: hidden;" to fix vertical as well.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.html#newco...
demos/racer/racer.html:24:
src="https://dart.googlecode.com/svn/branches/bleeding_edge/dart/client/dart.js">
Interesting, are we advised to use https for this now?  (authentication of
server identity?)

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart
File demos/racer/tire.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart#newcode3
demos/racer/tire.dart:3: // '_'.
Er, can't they?  (work for me)

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart#newcode53
demos/racer/tire.dart:53: final Vector currentRightNormal =
_body.getWorldVector(_worldLeft);
Future: Should we bug/investigate constant Vectors (or is that part of the new
Math library's goals?)

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart#newcod...
demos/racer/tire.dart:122: // manually.
I meant to ask last time but why not simply make it hashable?

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart#newcod...
demos/racer/tire.dart:126: final Vector _worldUp = null;
nit: I always feel register-like things need explaining (either by naming (e.g.
_tmpWorldLeft) or DartDoc).

[dominich, 2012-07-26 18:28:03]

Sigh. I generated the docs and JS without thinking. Please ignore doc/* and
*.js* in your next review.

Sorry for the pain.

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart
File demos/racer/racer.dart (right):

http://codereview.appspot.com/6440043/diff/1/demos/racer/racer.dart#newcode153
demos/racer/racer.dart:153: Car _car;
On 2012/07/26 16:36:56, rupertk wrote:
> On 2012/07/25 21:50:18, dominich wrote:
> > On 2012/07/25 20:04:59, rupertk wrote:
> > > Consistency: I quite like having members at the bottom but most of this
> > library
> > > puts at top.
> > 
> > Done.
> 
> Done?  (meaning decided against consistency with library but consistent with
> other racer files?)

Done, as in I moved them all to the bottom as that's what I prefer. I will do
the same throughout box2d :)

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart
File lib/common/MathBox.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/common/MathBox.dart#newcode20
lib/common/MathBox.dart:20: MathBox() { }
On 2012/07/26 16:36:56, rupertk wrote:
> On 2012/07/25 21:50:18, dominich wrote:
> > On 2012/07/25 20:04:59, rupertk wrote:
> > > Think we need a bug to fix this static-methods class!
> > > (doesn't need constructor and probably ought to be functions in non-class
> file
> > > that one pulls-in with namespace.)
> > 
> > that and get clamp and degToRad and radToDeg added to Dart core.
> 
> Will leave bugging this to you (since you know how to phrase better.)

clamp was already bugged.
dartbug.com/4211 covers degrees-to-radians.

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/World.dart
File lib/dynamics/World.dart (right):

http://codereview.appspot.com/6440043/diff/1/lib/dynamics/World.dart#newcode1140
lib/dynamics/World.dart:1140: }
On 2012/07/26 16:36:56, rupertk wrote:
> On 2012/07/25 21:50:18, dominich wrote:
> > On 2012/07/25 20:04:59, rupertk wrote:
> > > I'm sure you're right but curious what the problem was? (couldn't spot
> > > difference in cardemo.)
> > 
> > Trying to draw the boundary, which is an 'edge', failed as fill doesn't fill
> > shapes without an interior. We could change this to stroke and fill solid
> > polygons, and that would work too, but this seemed more generally correct.
> 
> hm, ok, will take your word for it.
> Disabling this change seems to produce no discernible difference in racer on
> mine.  It's only 1 extra comparison in debug-graphics :-)

Disabling it leads to the walls disappearing in mine. Weeird.

http://codereview.appspot.com/6440043/diff/13005/demos.html
File demos.html (right):

http://codereview.appspot.com/6440043/diff/13005/demos.html#newcode62
demos.html:62: menu.appendChild(item);
On 2012/07/26 16:36:56, rupertk wrote:
> Might it be neater to add 'racer/racer' to the 'demos' array? (seems to work)

It doesn't load the custom racer.html file but imports the racer.dart script.
What we should do is put some of the overflow stuff into demo.html, add a header
to that, and set the header/background color based on metadata per demo.

Another CL, i think.

http://codereview.appspot.com/6440043/diff/13005/demos/demo.dart
File demos/demo.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/demo.dart#newcode66
demos/demo.dart:66: : this.withGravity(new Vector(0, GRAVITY), viewportScale);
On 2012/07/26 16:36:56, rupertk wrote:
> Seems fine.
> I'm curious why you chose against optional parameter?  (my feeling is that it
> ought to make for a more digestible API.  However this is only demo code
so...)

Redirecting constructors don't support optional parameters, as far as i can
tell. If there's a better way to do this in Dart, i'm all ears.

http://codereview.appspot.com/6440043/diff/13005/demos/demo.dart#newcode81
demos/demo.dart:81: if (this.name != null) {
On 2012/07/26 16:36:56, rupertk wrote:
> Shouldn't this be !==

Done.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/car.dart
File demos/racer/car.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/racer/car.dart#newcode24
demos/racer/car.dart:24: final RevoluteJointDef jointDef = new
RevoluteJointDef();
On 2012/07/26 16:36:56, rupertk wrote:
> Checking my understanding: if we had WeldJoint implemented, this could use
that
> instead of a RevoluteJoint, right?
> 
> It seems like it's merely a body at an angle (that is dynamically set) to
> another body.  I guess with RJ and at the right scale (or desired simulation
> accuracy), collisions might twist the wheel from its chosen angle?

exactly - and they do. Try hitting the wall at high speed.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/car.dart#newcode53
demos/racer/car.dart:53: _frJoint = world.createJoint(jointDef);
On 2012/07/26 16:36:56, rupertk wrote:
> Per "welcome suggestions on how to improve it", best I could come up with
> (below) isn't great. 

Less repetitive, yes. More obscure about which constants go with which tire,
yes. Multiple boolean parameters to a method reducing readability, yes.

I'll stick with the current one unless you feel very strongly.

> Kind'a needs multi-value returns but even then...
> 
>     _blTire = _createTire(world, jointDef, false, true);
>     world.createJoint(jointDef);
>     _brTire = _createTire(world, jointDef, false, false);
>     world.createJoint(jointDef);
> 
>     _flTire = _createTire(world, jointDef, true, true);
>     _flJoint = world.createJoint(jointDef);
> 
>     _frTire = _createTire(world, jointDef, true, false);
>     _frJoint = world.createJoint(jointDef);
>   }
> 
>   Tire _createTire(final World world, final RevoluteJointDef jointDef, final
> bool front, final bool left) {
>     final num x = left ? -3 : 3;
>     num y, maxDriveForce, maxLateralImpulse;
>     if (front) {
>       y = 8.5;
>       maxDriveForce = frontTireMaxDriveForce;
>       maxLateralImpulse = frontTireMaxLateralImpulse;
>     } else {
>       y = 0.75;
>       maxDriveForce = backTireMaxDriveForce;
>       maxLateralImpulse = backTireMaxLateralImpulse;
>     }
>     final Tire tire = new Tire(world, maxForwardSpeed, maxBackwardSpeed,
>         maxDriveForce, maxLateralImpulse);
>     jointDef.bodyB = tire._body;
>     jointDef.localAnchorA.setCoords(x, y);
>     return tire;
>   }

http://codereview.appspot.com/6440043/diff/13005/demos/racer/car.dart#newcode78
demos/racer/car.dart:78: final double turnPerTimeStep = turnSpeedPerSec / 60.0;
On 2012/07/26 16:36:56, rupertk wrote:
> Actually, couldn't you just move all these out of the method now?

Done.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.dart
File demos/racer/racer.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.dart#newco...
demos/racer/racer.dart:22: String get name() => null;
On 2012/07/26 16:36:56, rupertk wrote:
> twitch, twitch

I was sobbing as I wrote that. In a future CL, I'll have the demo.dart not have
names to display in the canvas and instead I'll have the name in the .html file.

I might do the same with fps.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.dart#newco...
demos/racer/racer.dart:78: _createBoundary();
On 2012/07/26 16:36:56, rupertk wrote:
> Hm, why put these lower-level (more detailed) methods *above* the method that
> calls them?
> (I know JS-lint requires this but don't believe Dart does and I believe it's
> more common to flow 'down' for readability.)
> (perhaps for minimizing diff with previous?)

I haven't done enough due diligence on method ordering. I'll get on that.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.html
File demos/racer/racer.html (right):

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.html#newcode1
demos/racer/racer.html:1: <!DOCTYPE html>
On 2012/07/26 16:36:56, rupertk wrote:
> Not really this file but...
> How come you included the .js.map file but not the .js file?

I didn't do anything - rietveld refused to upload the .js :)

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.html#newcode2
demos/racer/racer.html:2: <html style="overflow-x: hidden">
On 2012/07/26 16:36:56, rupertk wrote:
> Sorry, realised it should have been "overflow: hidden;" to fix vertical as
well.

good point - i thought about it but decided vertical wouldn't be a problem. but
then i have a ridiculous monitor size.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.html#newco...
demos/racer/racer.html:24:
src="https://dart.googlecode.com/svn/branches/bleeding_edge/dart/client/dart.js">
On 2012/07/26 16:36:56, rupertk wrote:
> Interesting, are we advised to use https for this now?  (authentication of
> server identity?)

This is for local testing - long story short, my local machine's web server is
only accessible over https and so this avoids mixed content warnings. Once it's
up, it should be http so i'll change this.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart
File demos/racer/tire.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart#newcode53
demos/racer/tire.dart:53: final Vector currentRightNormal =
_body.getWorldVector(_worldLeft);
On 2012/07/26 16:36:56, rupertk wrote:
> Future: Should we bug/investigate constant Vectors (or is that part of the new
> Math library's goals?)

In another project I've been playing with the idea of constant vectors. It's a
tricky thing to get right but i suppose an explicit ConstVector type is the way
to go.

I might add it to the DartVectorMath library on a pull request and see how far
it goes.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart#newcod...
demos/racer/tire.dart:122: // manually.
On 2012/07/26 16:36:56, rupertk wrote:
> I meant to ask last time but why not simply make it hashable?

Done.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart#newcod...
demos/racer/tire.dart:126: final Vector _worldUp = null;
On 2012/07/26 16:36:56, rupertk wrote:
> nit: I always feel register-like things need explaining (either by naming
(e.g.
> _tmpWorldLeft) or DartDoc).

Done.

[rupertk, 2012-07-27 16:10:02]

2 final thoughts.
You know challenging me with "they don't but I'm all ears" is simply
irresistible ;-)

Btw, there should be 3 comments on this review.  Just checking I'm using
Rietveld correctly since some of my drafts seem to show against the previous
review version?!

I'll send the patch along separately.
R.

http://codereview.appspot.com/6440043/diff/13005/demos/demo.dart
File demos/demo.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/demo.dart#newcode66
demos/demo.dart:66: : this.withGravity(new Vector(0, GRAVITY), viewportScale);
On 2012/07/26 18:28:04, dominich wrote:
> On 2012/07/26 16:36:56, rupertk wrote:
> > Seems fine.
> > I'm curious why you chose against optional parameter?  (my feeling is that
it
> > ought to make for a more digestible API.  However this is only demo code
> so...)
> 
> Redirecting constructors don't support optional parameters, as far as i can
> tell. If there's a better way to do this in Dart, i'm all ears.

So redirecting constructors do /seem/ to support optional parameters (whether
they should = different question).
Whether they need to is debatable since the constructor one redirects to might
cover all optional parts.  (no, haven't fully thought this through but in this
case...)
It turns out a single constructor with optional parameters looks neatest [to my
eye]:

  Demo([Vector gravity, this.viewportScale = _VIEWPORT_SCALE])
      : bodies = new List<Body>() {
    bool doSleep = true;
    if (null === gravity) gravity = new Vector(0, GRAVITY);
    world = new World(gravity, doSleep, new DefaultWorldPool());
  }

Of course, once we get constant Vectors, we can push the default up, too.

I wrote a little test to confirm before (version from patch) and after (the
above) and placed in demos area.  I'll send this to you in a patch (either for
validation or inclusion -- though we may want to use as base for *actual* tests
so might want it in top-level ("dartbox2d/tests")).

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.dart
File demos/racer/racer.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/racer/racer.dart#newco...
demos/racer/racer.dart:22: String get name() => null;
On 2012/07/26 18:28:04, dominich wrote:
> On 2012/07/26 16:36:56, rupertk wrote:
> > twitch, twitch
> 
> I was sobbing as I wrote that. In a future CL, I'll have the demo.dart not
have
> names to display in the canvas and instead I'll have the name in the .html
file.
> 
> I might do the same with fps.

Yup, that's what I did :-)

http://codereview.appspot.com/6440043/diff/22008/demos/racer/tire.dart
File demos/racer/tire.dart (right):

http://codereview.appspot.com/6440043/diff/22008/demos/racer/tire.dart#newcode7
demos/racer/tire.dart:7: //_maxLateralImpulse = maxLateralImpulse,
Any reason for keeping these commented-out _ assignments?

[dominich, 2012-07-27 22:28:24]

http://codereview.appspot.com/6440043/diff/13005/demos/demo.dart
File demos/demo.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/demo.dart#newcode66
demos/demo.dart:66: : this.withGravity(new Vector(0, GRAVITY), viewportScale);
On 2012/07/27 16:10:02, rupertk wrote:
> On 2012/07/26 18:28:04, dominich wrote:
> > On 2012/07/26 16:36:56, rupertk wrote:
> > > Seems fine.
> > > I'm curious why you chose against optional parameter?  (my feeling is that
> it
> > > ought to make for a more digestible API.  However this is only demo code
> > so...)
> > 
> > Redirecting constructors don't support optional parameters, as far as i can
> > tell. If there's a better way to do this in Dart, i'm all ears.
> 
> So redirecting constructors do /seem/ to support optional parameters (whether
> they should = different question).
> Whether they need to is debatable since the constructor one redirects to might
> cover all optional parts.  (no, haven't fully thought this through but in this
> case...)
> It turns out a single constructor with optional parameters looks neatest [to
my
> eye]:
> 
>   Demo([Vector gravity, this.viewportScale = _VIEWPORT_SCALE])
>       : bodies = new List<Body>() {
>     bool doSleep = true;
>     if (null === gravity) gravity = new Vector(0, GRAVITY);
>     world = new World(gravity, doSleep, new DefaultWorldPool());
>   }
> 
> Of course, once we get constant Vectors, we can push the default up, too.
> 
> I wrote a little test to confirm before (version from patch) and after (the
> above) and placed in demos area.  I'll send this to you in a patch (either for
> validation or inclusion -- though we may want to use as base for *actual*
tests
> so might want it in top-level ("dartbox2d/tests")).

Done.

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart
File demos/racer/tire.dart (right):

http://codereview.appspot.com/6440043/diff/13005/demos/racer/tire.dart#newcode3
demos/racer/tire.dart:3: // '_'.
On 2012/07/26 16:36:56, rupertk wrote:
> Er, can't they?  (work for me)

Done.

http://codereview.appspot.com/6440043/diff/22008/demos/racer/tire.dart
File demos/racer/tire.dart (right):

http://codereview.appspot.com/6440043/diff/22008/demos/racer/tire.dart#newcode7
demos/racer/tire.dart:7: //_maxLateralImpulse = maxLateralImpulse,
On 2012/07/27 16:10:02, rupertk wrote:
> Any reason for keeping these commented-out _ assignments?

Testing and didn't cleanup. Good spot.

[rupertk, 2012-07-28 00:27:13]

LGTM