Improve kinematics for compass

[pixelzoom]

Related to #103 code review...

In our code-review kickoff meeting, @matthew-blackman noted that sometimes the compass needle does not wobble. This could be problematic for lessons that assign signficance to the wobble, like this one:

The reason that the needle does not wobble sometimes is that there is a wobble threhold, below which the compass snaps to the field vector angle. If the change in angle per time step happens to move the needle to an angle below this threshold, there will be no wobble.

@matthew-blackman suggested removing the wobble threshold, and that damping should take care of it. An initial test with this change in KinemeticCompass.ts looked promising:

// Angle at which the needle stops wobbling and snaps to the actual field orientation.
- const WOBBLE_THRESHOLD = Utils.toRadians( 0.2 );
+ const WOBBLE_THRESHOLD = Utils.toRadians( 0 );

My concern with this change is that the compass will continue to wobble for much longer than is actually visible, doing unnecessary work.

[pixelzoom]

My concern with this change is that the compass will continue to wobble for much longer than is actually visible, doing unnecessary work.

This concern has been confirmed. The compass continues to wobble long after it is no longer visibly wobbling -- still wobbling very small amounts after several minutes.

[pixelzoom]

Reducing WOBBLE_THRESHOLD significantly reduces (but does not eliminate) the probability that the needle will not wobble, while ensuring that the needle comes to rest. This value causes the needle to come to rest ~2 seconds after the needle is no longer visibly wobbling:

- const WOBBLE_THRESHOLD = Utils.toRadians( 0.2 );
+ const WOBBLE_THRESHOLD = Utils.toRadians( 0.01 );

@matthew-blackman please test-drive and let me know if this adresses your concern.

[matthew-blackman]

This is behaving well in my testing, and I think it's a major improvement on the behavior of the needle. Nice work!

It seems like there may still be rare cases in which the needle happens to fall within the WOBBLE_THRESHOLD while rotating, causing it to stop abruptly. Would it be worth considering a threshold angular speed that is included in the immediate stop logic?

I was considering recommending a change in KinematicCompass.ts from

 if ( Math.abs( deltaAngle ) < WOBBLE_THRESHOLD )
// stop the needle

to

 if ( Math.abs( deltaAngle ) < WOBBLE_THRESHOLD && Math.abs( angularSpeed ) < ANGULAR_SPEED_THRESHOLD
// stop the needle

What do you think @pixelzoom?

[pixelzoom]

I think that's a good idea, and I've implemented it in the above commit. @matthew-blackman please review.

Here's the new threshold. I arrived at the value by adding a console.log and observing the value of angularVelocityProperty when a nice wobble occurs.

// To ensure that the needle wobbles before it snaps to the actual field angle.
// See https://github.com/phetsims/faradays-electromagnetic-lab/issues/108
const ANGULAR_VELOCITY_THRESHOLD = Utils.toRadians( 0.5 );

Since our discussion about this has involved both "angular speed" and "angular velocify", I'm wondering if I should make any change there. Since it's a Hollywood model, we could rightfully claim that it's either. Mike Dubson specified the orginal algorithm for this, in which he used "omega", the symbol for angular velocity. And if we change to angularSpeedProperty, what becomes of angularAccelerationProperty? @matthew-blackman thoughts? Do you see any compelling reason to change?

[pixelzoom]

Since our discussion about this has involved both "angular speed" and "angular velocify", I'm wondering if I should make any change there.

Thinking about this more, I don't think a change is needed.

Angular velocity is correct. We are computing: ( change in angle ) / dt. It's in radians/sec, and it's a signed quantity.

Angular speed is not in angular units, and it is unsigned. It is Math.abs( angular velocity ) * R, where R is the radius, the distance from the center point about which we are rotating. If our distance units were meters, it would be in meters/sec.

[matthew-blackman]

The behavior is looking really good. I updated a variable name and some documentation in the commit. I added a note that we are taking the magnitude of the angular velocity when comparing to the threshold to make things more clear. Please review and feel free to close if you approve.

[pixelzoom]

Thanks but... WOBBLE_THRESHOLD has existed since 2004, it's how Mike Dubson and I talked about it, and I needed zero prompting to recall that when I was porting the Java code. If I ever need to touch it again, I will likely be looking for "wobble". So I'll keep your new doc, but I'd like to revert the name change.

[pixelzoom]

I did not revert to WOBBLE_THRESHOLD, because there are now 3 constants that control the wobble. I did rename ANGULAR_DISPLACEMENT_THRESHOLD to DELTA_ANGLE_THRESHOLD, because the code is:

 if (
          // If the needle is close enough to the field angle and rotating slowly enough, snap it to the field angle.
          // See https://github.com/phetsims/faradays-electromagnetic-lab/issues/108.
          ( Math.abs( deltaAngle ) < DELTA_ANGLE_THRESHOLD && Math.abs( this.angularVelocityProperty.value ) < ANGULAR_VELOCITY_THRESHOLD ) ||

... and I think the constants should match the things to which they are being compared.

I also simplified the documentation of DELTA_ANGLE_THRESHOLD and ANGULAR_VELOCITY_THRESHOLD. No matter how many times I read the new doc, I got confused. So I think it's better to just say "Threshold for {something} that is used to decide whether the needle should stop wobbling." and expect the reader to see how it's used if they want specifics.

I think this is ready to close, reopen if I missed something.