Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CT: assertion failure in Graph.computeWindingMap #72

Closed
sarchang opened this issue Jul 2, 2021 · 32 comments
Closed

CT: assertion failure in Graph.computeWindingMap #72

sarchang opened this issue Jul 2, 2021 · 32 comments

Comments

@sarchang
Copy link
Contributor

sarchang commented Jul 2, 2021

Uncaught Error: Assertion failed
    at window.assertions.assertFunction (assert.js:25)
    at Graph.computeWindingMap (Graph.js:1091)
    at Graph.computeSimplifiedFaces (Graph.js:219)
    at Function.intersectionNonZero (Graph.js:1341)
    at Function.intersection (Shape.js:2239)
    at Spotlight.getIntersection (Spotlight.js:151)
    at Spotlight.js:47
    at getDerivedValue (DerivedProperty.js:30)
    at DerivedProperty.getDerivedPropertyListener (DerivedProperty.js:112)
    at TinyProperty.emit (TinyEmitter.js:86)
@veillette
Copy link
Contributor

Still present

geometric-optics : xss-fuzz
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/geometric-optics/geometric-optics_en.html?continuousTest=%7B%22test%22%3A%5B%22geometric-optics%22%2C%22xss-fuzz%22%5D%2C%22snapshotName%22%3A%22snapshot-1625753637307%22%2C%22timestamp%22%3A1625756120390%7D&brand=phet&ea&fuzz&stringTest=xss&memoryLimit=1000
Query: brand=phet&ea&fuzz&stringTest=xss&memoryLimit=1000
Uncaught Error: Assertion failed
Error: Assertion failed
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/assert/js/assert.js:25:13)
at Graph.computeWindingMap (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/kite/js/ops/Graph.js:1091:19)
at Graph.computeSimplifiedFaces (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/kite/js/ops/Graph.js:219:10)
at Function.intersectionNonZero (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/kite/js/ops/Graph.js:1341:11)
at Function.intersection (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/kite/js/Shape.js:2239:18)
at Spotlight.getIntersection (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/geometric-optics/js/lens/model/Spotlight.js:151:18)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/geometric-optics/js/lens/model/Spotlight.js:47:21
at getDerivedValue (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/axon/js/DerivedProperty.js:30:10)
at DerivedProperty.getDerivedPropertyListener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/axon/js/DerivedProperty.js:112:18)
at TinyProperty.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1625753637307/axon/js/TinyEmitter.js:86:9)
id: Bayes Chrome
Snapshot from 7/8/2021, 10:13:57 AM

@veillette
Copy link
Contributor

veillette commented Jul 8, 2021

It fails at this point in computeWindingMap in Graph.js:

1459        assert && assert( forwardFace !== reversedFace );
  /**
   * Computes the winding map for each face, starting with 0 on the unbounded face (for each shapeId).
   * @private
   */
  computeWindingMap() {
    const edges = this.edges.slice();

    // Winding numbers for "outside" are 0.
    const outsideMap = {};
    for ( let i = 0; i < this.shapeIds.length; i++ ) {
      outsideMap[ this.shapeIds[ i ] ] = 0;
    }
    this.unboundedFace.windingMap = outsideMap;

    // We have "solved" the unbounded face, and then iteratively go over the edges looking for a case where we have
    // solved one of the faces that is adjacent to that edge. We can then compute the difference between winding
    // numbers between the two faces, and thus determine the (absolute) winding numbers for the unsolved face.
    while ( edges.length ) {
      for ( let j = edges.length - 1; j >= 0; j-- ) {
        const edge = edges[ j ];

        const forwardHalf = edge.forwardHalf;
        const reversedHalf = edge.reversedHalf;

        const forwardFace = forwardHalf.face;
        const reversedFace = reversedHalf.face;
        assert && assert( forwardFace !== reversedFace );

        const solvedForward = forwardFace.windingMap !== null;
        const solvedReversed = reversedFace.windingMap !== null;

        if ( solvedForward && solvedReversed ) {
          edges.splice( j, 1 );

          if ( assert ) {
            for ( let m = 0; m < this.shapeIds.length; m++ ) {
              const id = this.shapeIds[ m ];
              assert( forwardFace.windingMap[ id ] - reversedFace.windingMap[ id ] === this.computeDifferential( edge, id ) );
            }
          }
        }
        else if ( !solvedForward && !solvedReversed ) {
          continue;
        }
        else {
          const solvedFace = solvedForward ? forwardFace : reversedFace;
          const unsolvedFace = solvedForward ? reversedFace : forwardFace;

          const windingMap = {};
          for ( let k = 0; k < this.shapeIds.length; k++ ) {
            const shapeId = this.shapeIds[ k ];
            const differential = this.computeDifferential( edge, shapeId );
            windingMap[ shapeId ] = solvedFace.windingMap[ shapeId ] + differential * ( solvedForward ? -1 : 1 );
          }
          unsolvedFace.windingMap = windingMap;
        }
      }
    }
  }

@veillette
Copy link
Contributor

veillette commented Jul 8, 2021

This happens rather irregularly on Bayes. My initial assumption is that it is a corner case.

Possibly when the spotlight gets so small that it becomes a point on the edge of the projector screen. This would create an a graph union that could potentially fail. This is pure speculation on my part.

I have not been able to reproduce it.

@phet-steele
Copy link

phet-steele commented Jul 9, 2021

Possibly when the spotlight gets some small that it becomes a point on the edge of the projector screen. This would create an a graph union that could potentially fail. This is pure speculation on my part.

In a setup like below, I can at least get the sim to freeze up (from poor performance, I assume), but I get no error. Until such time that I determine if this screenshot is at all related to this issue, or this is its own issue, I will just leave it here. I got this on Win 10 FF.

image

@veillette
Copy link
Contributor

I tried to setup cases where the intersections of two shapes might fail, say a circle "kissing" a rectangle

    const shape1 = Shape.ellipse( 80, 150, 20, 20 );
    const shape2 = Shape.rectangle( 100, 100, 100, 100 );

    const intersection = Shape.intersection( [ shape2, shape1 ] );

but that does not trigger an assertion.

@veillette
Copy link
Contributor

Even the case of disk of radius 0 on the edge of a rectangle is handled correctly.

    const shape1 = Shape.ellipse( 100, 150, 0, 0 );
    const shape2 = Shape.rectangle( 100, 100, 100, 100 );

    const intersection = Shape.intersection( [ shape2, shape1 ] );

@veillette
Copy link
Contributor

@jonathanolson , I think this is a Kite bug that involves Shape.intersection.

We are passing two shapes and want to determine their intersections.

  /**
   * @private
   * @param {Vector2} screenPosition
   * @param {Vector2} opticPosition
   * @param {number} opticDiameter
   * @param {Vector2} targetPosition
   * @returns {Shape}
   */
  getDiskShape( screenPosition, opticPosition, opticDiameter, targetPosition ) {
    const diskPosition = this.getDiskPosition( screenPosition, opticPosition, targetPosition );

    assert && assert( diskPosition.isFinite(), 'disk Position is not finite' );

    const radiusY = this.getDiskRadius( screenPosition, opticPosition, opticDiameter - OPTICAL_ELEMENT_TIP_OFFSET, targetPosition );

    assert && assert( isFinite( radiusY ), 'y radius is not finite' );

    // ellipse width is half the height to give an approximation of 3D effect
    const radiusX = 1 / 2 * radiusY;
    return Shape.ellipse( diskPosition.x, diskPosition.y, radiusX, radiusY, 2 * Math.PI );
  }

  /**
   * @private
   * @param {Vector2} screenPosition
   * @param {Vector2} opticPosition
   * @param {number} opticDiameter
   * @param {Vector2} targetPosition
   * @returns {Shape}
   */
  getIntersection( screenPosition, opticPosition, opticDiameter, targetPosition ) {
    return Shape.intersection(
      [ this.getDiskShape( screenPosition, opticPosition, opticDiameter, targetPosition ),
        this.getScreenShape() ] );
  }

ScreenShape is a fixed, well defined parallelogram.
DiskShape changes and could give rise to ellipses of different positions and radii.

I added some assertions to diskShape to make sure its shape is legit. However, I have not been able to trigger any assertions within geometric optics itself. The assertion that is triggered is within kite. I must admit that I dont understand what computeWindingMap is doing.

It would be useful to get some insight into what is going on.

Unfortunately, even though this assertion is triggered on a rather regular basis on Bayes, we dont have very clear procedure to trigger it.

@veillette
Copy link
Contributor

Assigning @jonathanolson to look into it.

veillette added a commit that referenced this issue Jul 21, 2021
@veillette
Copy link
Contributor

I added assertions to make sure that the two shapes that are passed are appropriate. We will monitor Bayes to see if that proves useful.

@veillette
Copy link
Contributor

Using BinaryResult instead of the more general intersection method still trigger assertions.
This occurs despite filtering the zero area shape to BinaryResult.

https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/geometric-optics/geometric-optics_en.html?continuousTest=%7B%22test%22%3A%5B%22geometric-optics%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1626991273286%22%2C%22timestamp%22%3A1627008770681%7D&brand=phet&ea&fuzz&memoryLimit=1000
Query: brand=phet&ea&fuzz&memoryLimit=1000
Uncaught Error: Assertion failed
Error: Assertion failed
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/assert/js/assert.js:25:13)
at Function.intersect (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/kite/js/ops/BoundsIntersection.js:213:17)
at Function.intersect (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/kite/js/segments/Segment.js:739:33)
at Graph.eliminateIntersection (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/kite/js/ops/Graph.js:694:41)
at Graph.computeSimplifiedFaces (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/kite/js/ops/Graph.js:189:10)
at Function.binaryResult (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/kite/js/ops/Graph.js:1286:11)
at Spotlight.getIntersection (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/geometric-optics/js/lens/model/Spotlight.js:170:20)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/geometric-optics/js/lens/model/Spotlight.js:48:21
at getDerivedValue (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/axon/js/DerivedProperty.js:30:10)
at DerivedProperty.getDerivedPropertyListener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1626991273286/axon/js/DerivedProperty.js:112:18)
id: Bayes Chrome
Snapshot from 7/22/2021, 6:01:13 PM

@jonathanolson
Copy link
Contributor

(I will be looking into this)

@kathy-phet
Copy link

This is really reproducible ...
From the lens screen
Select Light
Switch to principle rays
Use the slider to reduce the refractive index. It seems to happen every time to me.

@veillette
Copy link
Contributor

Thanks @kathy-phet.
I was able to reproduce.

Reduce the slider until you reach the index of refraction 1.36.

image

@veillette
Copy link
Contributor

In the above case, it is the "second source" spotlight that fails.

@jonathanolson
Copy link
Contributor

I tried reproducing using the above instructions in master, and couldn't reproduce. I tried fuzzing for 30 minutes, both with master and a dev version (https://phet-dev.colorado.edu/html/geometric-optics/1.0.0-dev.9/phet/geometric-optics_all_phet_debug.html?fuzz). Thoughts on how I can reproduce? Is it potentially dependent on screen size?

Alternatively, if you could toString() both of the shapes being intersected and report that, I could presumably reproduce the kite error from that.

@jonathanolson jonathanolson removed their assignment Aug 5, 2021
@veillette
Copy link
Contributor

We have moved the initial position of the source since, and this affects the reproduction instructions.

@veillette
Copy link
Contributor

I tried to co back to our previous initial positions for the sources but unfortunately we also changed the size and position of the projector screen during a refactor, so I have not been able to reproduce it.

@veillette
Copy link
Contributor

Considering the comment from @jonathanolson , I'll attempt to go back to a previous commit where KP was able to reproduce it by merely moving the sliders.

@pixelzoom
Copy link
Contributor

Since this is not in the prototype, unassigning until I start working on public production issues.

@pixelzoom
Copy link
Contributor

Note that Spotlight was renamed LightSpot, and SpotlightNode was renamed LightSpotNode, per #196.

@pixelzoom
Copy link
Contributor

Still failing occassionally in CT:

geometric-optics : multitouch-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/geometric-optics/geometric-optics_en.html?continuousTest=%7B%22test%22%3A%5B%22geometric-optics%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1634648928056%22%2C%22timestamp%22%3A1634651589847%7D&brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=false
Query: brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=false
Uncaught Error: Assertion failed
Error: Assertion failed
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/assert/js/assert.js:25:13)
at Graph.computeWindingMap (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/kite/js/ops/Graph.js:1222:15)
at Graph.computeSimplifiedFaces (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/kite/js/ops/Graph.js:339:10)
at Function.binaryResult (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/kite/js/ops/Graph.js:1406:11)
at LightSpot.getIntersection (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/geometric-optics/js/lens/model/LightSpot.js:187:20)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/geometric-optics/js/lens/model/LightSpot.js:52:14
at getDerivedValue (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/axon/js/DerivedProperty.js:30:10)
at DerivedProperty.getDerivedPropertyListener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/axon/js/DerivedProperty.js:113:18)
at TinyProperty.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/axon/js/TinyEmitter.js:86:9)
at DerivedProperty._notifyListeners (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1634648928056/axon/js/Property.js:279:23)
id: Bayes Chrome
Snapshot from 10/19/2021, 7:08:48 AM

@pixelzoom pixelzoom changed the title Spotlight assertion failures Light spot assertion failures Oct 20, 2021
@pixelzoom pixelzoom changed the title Light spot assertion failures LightSpot assertion failures Oct 20, 2021
@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 11, 2021

We'll need @jonathanolson to take at look at this. Like #163, this is a problem in kite.Graph.

@pixelzoom pixelzoom changed the title LightSpot assertion failures CT: assertion failure in Graph.computeWindingMap Jan 4, 2022
@pixelzoom
Copy link
Contributor

I'm going to close this because:

(1) I haven't seen this in CT since 10/19/21, almost 4 months ago.
(2) @kathy-phet's steps in #72 (comment) no longer reproduce the problem.
(3) So much code has been rewritten, reorganized, renamed, etc. that the problem may no longer exist.

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 2, 2022

Reopening. While this problem is rare, it's not resolved. I hit it while running the State Wrapper for ~5 minutes in my local copy with ?sim=geometric-optics&phetioDebug&fuzz Unlikely that it's related to the State Wrapper.

@pixelzoom pixelzoom reopened this Mar 2, 2022
@pixelzoom pixelzoom removed type:bug Something isn't working dev:code-review labels Mar 14, 2022
@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 17, 2022

I fuzzed tested locally for 30 minutes and did not encounter this problem.

Test URLs:

  • geometric-optics_en.html?brand=phet&ea&debugger&fuzz
  • phet-io-wrappers/state/?sim=geometric-optics&phetioDebug&fuzz

@pixelzoom
Copy link
Contributor

geometric-optics : fuzz : unbuilt : fuzzLight
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/geometric-optics/geometric-optics_en.html?continuousTest=%7B%22test%22%3A%5B%22geometric-optics%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22fuzzLight%22%5D%2C%22snapshotName%22%3A%22snapshot-1648925503062%22%2C%22timestamp%22%3A1648951254249%7D&brand=phet&ea&fuzz&memoryLimit=1000&screens=1&scene=light
Query: brand=phet&ea&fuzz&memoryLimit=1000&screens=1&scene=light
Uncaught Error: Assertion failed
Error: Assertion failed
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/assert/js/assert.js:25:13)
at Graph.computeWindingMap (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/chipper/dist/js/kite/js/ops/Graph.js:1425:19)
at Graph.computeSimplifiedFaces (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/chipper/dist/js/kite/js/ops/Graph.js:306:10)
at Function.binaryResult (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/chipper/dist/js/kite/js/ops/Graph.js:1627:11)
at getLightSpotShape (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/chipper/dist/js/geometric-optics/js/common/model/LightSpot.js:106:16)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/chipper/dist/js/geometric-optics/js/common/model/LightSpot.js:43:293
at getDerivedValue (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/chipper/dist/js/axon/js/DerivedProperty.js:26:10)
at DerivedProperty.getDerivedPropertyListener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/chipper/dist/js/axon/js/DerivedProperty.js:112:17)
at TinyProperty.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/chipper/dist/js/axon/js/TinyEmitter.js:68:9)
at DerivedProperty._notifyListeners (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1648925503062/chipper/dist/js/axon/js/Property.js:218:23)
id: Bayes Chrome
Snapshot from 4/2/2022, 12:51:43 PM

@pixelzoom
Copy link
Contributor

This is a non-blocking error. It won't cause the sim to crash, and is unlikely to result in any noticeable problems in practice. 1.1 was published with this as a known issue.

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 14, 2022

In c8f18cd, the implementation changed. LightSpot.ts no longer computes intersection with the projection screen, getLightSpotShape no longer exists, and Graph.computeWindingMap is therefore never called. So this issue is no irrelevant and can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants