Free-Form and discontinuities with small numberOfPoints

[veillette]

This issue becomes more prominent as the numberOfPoints is lowered.

While dragging (slowly) with free form, one sees only discontinuities.
for numberOfPoints=50, we can observe

Repeating with the default number of points, we can also generate discontinuities, if dragging very, very slowly. (It is much harder to reproduce, one must drag very slowly)

[veillette]

The free form below on the left was done one continuous drag (which lasted about ten seconds).

[veillette]

The problem lies with the way we go about assigning discontinuities.
For free form, we deal with three positions:

  private drawFreeformToPosition( position: Vector2,
                                  penultimatePosition: Vector2 | null,
                                  antepenultimatePosition: Vector2 | null ): void {

but we assign the discontinuities with

      if ( lastPoint.x > closestPoint.x ) {
        this.getClosestPointAt( position.x - this.deltaX ).pointType = 'discontinuous';
      }
      else {
        this.getClosestPointAt( position.x + this.deltaX ).pointType = 'discontinuous';
      }

deltaX is the minimum separation between two points. As a result, position.x + this.deltaX might fall outside of the scope of position, penultimatePosition, and antepenultimatePosition. This explains, why this becomes more apparent when deltaX is large (by decreasing numberOfPoints) or by moving the dragHandler very slowly (such that position, penultimatePosition and antepenultimatePosition are nearly the same.

[veillette]

The way forward is to assign the type after we assign the y values.
In addition, we need to keep track of the previously assigned type at the edges, so we can assign these points to smooth.

[veillette]

Some progress. The following works as long as the drag is unidirectional.

    // assignType
    if ( penultimatePosition ) {

      const lastPoint = this.getClosestPointAt( penultimatePosition.x );

      const lastPointIndex = this.getIndex( lastPoint );
      const closestPointIndex = this.getIndex( closestPoint );

      let min = Math.min( closestPointIndex, lastPointIndex );
      let max = Math.max( closestPointIndex, lastPointIndex );
      for ( let i = min; i <= max; i++ ) {
        this.points[ i ].pointType = 'smooth';
      }

      if ( antepenultimatePosition ) {

        // Point associated with the last drag event
        const nextToLastPoint = this.getClosestPointAt( antepenultimatePosition.x );

        const nextToLastPointIndex = this.getIndex( nextToLastPoint );

        min = Math.min( closestPointIndex, nextToLastPointIndex );
        max = Math.max( closestPointIndex, nextToLastPointIndex );

        for ( let i = min; i <= max; i++ ) {
          this.points[ i ].pointType = 'smooth';
        }
      }

      closestPoint.pointType = 'discontinuous';
      if ( lastPointIndex > closestPointIndex ) {
        this.getClosestPointAt( closestPoint.x - this.deltaX ).pointType = 'discontinuous';
      }
      else if ( lastPointIndex < closestPointIndex ) {
        this.getClosestPointAt( closestPoint.x + this.deltaX ).pointType = 'discontinuous';
      }
    }

We need to handle the case where the drag changes direction.

[veillette]

I committed the piece of code above about the tagging of pointType. There is more work to do on it, which I plan to finish today.

[veillette]

Hmm, I have resolved the issue but code-wise, I am not very happy with the result. It has gotten too complicated.
Even with three drag points to keep track of, there are many cases to handle.

• are there one, two or three drag events
• if there are two: is the drag going to the left or to the right
• if there are three: is the drag turning around or unidirectional
• are any of three points sharing the same x values

I'll need to simplify the logic.

[veillette]

I need to add more documentation that goes beyond explaining the logic but the overall objective of tagging the pointType.
That would go a long way about explaining the logic of the code.

[veillette]

Given that we just finished a dev test (phetsims/qa#921) I think it is safe to leave the code base as it is. I added some documentation for future maintainers.