Is infinite Undo really necessary?

[pixelzoom]

The design document specifies "unlimited" undo. Anything "unlimited" is a red flag in programming.

Each undo requires saving the state in memory, before the user performs an operation. That state is currently 700 or more points for each undo. We could conceivable invent a more compact way to save state, but that's not the point. We have finite memory. So supporting unlimited undo is technically impossible - if you use the sim long enough without resetting, you will run out of memory, and the sim will crash.

It's also questionable whether unlimited undo is really needed. Will someone really do (say) 1000 operations, then want to undo them one by one? That seems unlikely - they are more likely to press the reset button and start over. What seems more likely is that the user will want to undo some number of recent operations, probably similar to the number of operations that they could recall in their memory. So saving a healthy number of undos (50? 100?) should not impact usability in practice.

My recommendation is to set a limit on the number of undos, starting with 50.

@amanda-phet @kathy-phet @veillette thoughts?

[amanda-phet]

We used to have this capped at 50. It looks like there was a discussion in June 2020:

Is 50 really necessary? I think we can limit this to 1-5 actions, and the button will go inactive when we can no longer undo. Most users will likely not expect the button to undo a whole lot. BL says we can do infinite, so we’ll do that.

I somewhat recall this conversation, and thinking it's unlikely someone needs to be able to undo that much, but it seemed easy enough to allow it and we couldn't think of any harm in doing so. It sounds like now that the curve is a discrete number of points, there is harm. So I think realistically, even 50 undo actions is more than necessary. If it's not a memory concern, then let's do 50. If it would save memory to have fewer undo actions available, I would be happy going down to 20.

[pixelzoom]

I agree that 20 is likely sufficient. And I doubt that 50 would cause a memory problem. @veillette let's make sure that there is a constant in CalculusGrapherConstants for configuring this. And I don't think it needs to be changeable via query parameter or PhET-iO.

[veillette]

A MAX_UNDO constant was created in CalculusGrapherConstants.

 /**
   * Saves the current y-value of the Point for the next undoToLastSave() method.
   * This method is invoked when the user finishes manipulating the OriginalCurve. When the undo button is pressed,
   * the Points of the OriginalCurve will be set to their last saved state.
   */
  public save(): void {

    // Save the current y-value of the CurvePoint.
    this.savedYValues.push( this.y );

    // empty first element of array if the number of saved values exceed MAX_UNDO
    while ( this.savedYValues.length > CalculusGrapherConstants.MAX_UNDO ) {

      // remove first value from array
      this.savedYValues.shift();
    }

  }

The saved method is now responsible for saving the Y value in an array and removing the first element of the array if it exceeds the limit.

[veillette]

We are not saving the full state of CurvePoint, since there is more to it than just merely saving the yValue.

Tracking the problem about saving the full state of CurvePoint in https://github.com/phetsims/calculus-grapher/issues/91

[veillette]

We have limited the number of undo to MAX_UNDO which is created in CalculusGrapherConstants. Closing.