phetsims / graphing-quadratics

"Graphing Quadratics" is an educational simulation in HTML5, by PhET Interactive Simulations.
MIT License
1 stars 4 forks source link

Missing dependency for DerivedProperty #205

Closed pixelzoom closed 10 months ago

pixelzoom commented 12 months ago

Related to https://github.com/phetsims/axon/issues/441 ...

In https://github.com/phetsims/graphing-quadratics/commit/0b01989eebe1859c8eab87872be2120b86cd05da, @samreid added accessNonDependencies: true to temporarily silence a missing dependency for a DerivedProperty. It should be addressed.

pixelzoom commented 11 months ago

The failure is in NoRealRootsNode centerProperty, relevant code below. I can't identify the missing dependency, and if I add a debugger to ReadOnlyProperty get, then I don't end up in this code. It looks like I'm ending up in something related to Node boundsProperty. And if I remove the dependency on this.boundsProperty, the error goes away. So I suspect that there's something weird going on here, possibly a problem with the algorithm for identifying dependencies, and would like to have @samreid take a look at this with me synchronously.

    // The center of this Node, typically at the origin, except when that would overlap the vertex's coordinates.
    // In that case, position above or below the x-axis, depending on which way the parabola opens.
    // See https://github.com/phetsims/graphing-quadratics/issues/88
    const centerProperty = new DerivedProperty(
      [ vertexVisibleProperty, coordinatesVisibleProperty, quadraticProperty, this.boundsProperty ],
      ( vertexVisible, coordinatesVisible, quadratic, bounds ) => {
        if ( vertexVisible && // the Vertex checkbox is checked
             coordinatesVisible && // the Coordinates checkbox is checked
             ( quadratic.roots && quadratic.roots.length === 0 ) && // no roots
             // vertex is in a position where its coordinates will overlap
             ( quadratic.vertex && vertexOverlapBounds.containsPoint( quadratic.vertex ) ) ) {

          // center above or below the x-axis, y offset determined empirically
          const y = quadratic.vertex.y + ( quadratic.a > 0 ? -Y_OFFSET : Y_OFFSET );
          return modelViewTransform.modelToViewXY( 0, y );
        }
        else {

          // center at the origin
          return modelViewTransform.modelToViewXY( 0, 0 );
        }
      }, {
        // accessNonDependencies: true //TODO https://github.com/phetsims/graphing-quadratics/issues/205
      }
    );
pixelzoom commented 10 months ago

Stack trace indicates that this is failing in Node.ts at line 1514. And the comment at the end of line 1514 looks suspcious.

        // TODO: consider changing to parameter object (that may be a problem for the GC overhead) https://github.com/phetsims/scenery/issues/1581
        if ( !ourBounds.equalsEpsilon( oldBounds, notificationThreshold ) ) {
1514       this.boundsProperty.notifyListeners( oldBounds ); // RE-ENTRANT CALL HERE, it will validateBounds()
        }
pixelzoom commented 10 months ago

The problem was that changing centerProperty = new DerivedProperty result in a change to this.boundsProperty, which cause Node.validateBounds to be called recusively (see the "RE-ENTRANT CALL HERE" comment above), which involved additional Node Properties. I avoid this by replacing centerProperty with a Multilink that sets this.center.

Closing.