-
Notifications
You must be signed in to change notification settings - Fork 5
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: cannot normalize a zero-magnitude vector #178
Comments
Here's the call to // direction of a ray to the top of the optic
const topDirection = topPoint.minus( sourcePosition ).normalized(); @arouinfar should this be addressed for the prototype? I have not investigated whether it causes the sim to crash in practice. |
@pixelzoom this seems a bit different than the assertion failures we passed over in #215. If it's a rare issue or doesn't cause a built sim to crash, I'm fine with deferring for the prototype. Perhaps we should ask QA to try and reproduce? What do would you recommend @pixelzoom? |
It's almost certainly a sim logic error, and non-fatal. While I'm not 100% certain, I'm fairly confident that this issue will not occur in practice, and can be deferred for the prototype. It occurs in other sims in CT, and we've (so far) ignored it in production deploys for those sims. So... Unassigning until post-prototype. |
I was never able to reproduce this error, and it has not occurred in CT for many months (almost 4 months?) And now that the source object cannot be moved as close to the optic, I'm confident that the offending vector can no longer have zero magnitude. So I'm going to close this issue. We can reopen if it reappears. |
Reopening, this is back. Mostly showng up in geometric-optics-basics:
|
I fuzzed tested the unbuilt sim for 30 minutes and did not encounter this problem. |
|
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. |
Can you confirm if this issue is still needed? I don't see it in CT right now, but it is noted as rare. For phetsims/qa#832 |
Yes, please keep this open. |
It is not necessary to address this for #278, the 1.3 PhET-iO release. |
This recurred in the 9/21/2023 5:41:05pm column. At this week's iteration planning meeting, we agreed we have an iteration goal to address CT errors this iteration. Can/should we remove the deferred label from this? Who should take the lead? |
This happens rarely. But I'll have a look. |
I'm unable to reproduce locally after 20 minutes of fuzzing. And it's unclear how this error can ever occur. The vector that's reported as having zero magnitude is const objectOpticVector = optic.positionProperty.value.minus( opticalObjectPosition ); So for MIN_DISTANCE_FROM_OBJECT_TO_OPTIC: 40, // cm In the above commit, I added this more detailed assertion in LightRays, and I'll will wait for the next failure in CT before doing further investigation: const objectOpticVector = optic.positionProperty.value.minus( opticalObjectPosition );
+ //TODO https://github.com/phetsims/geometric-optics/issues/178
+ assert && assert( objectOpticVector.magnitude !== 0,
+ `unexpected zero-magnitude vector: objectOpticVector=${objectOpticVector}, ` +
+ `optic.position=${optic.positionProperty.value}, opticalObjectPosition=${opticalObjectPosition}` ); |
Nothing in CT for 5 days, and I cannot reproduce this locally. Since this happens rarely, it's unlikely to be masking other problems, and I'd like to keep this issue open. But I'm going to unassigned until this sim is worked on again. |
Still occurring occassionally:
And here's the failure location in LightRays: 172 directions.push( firstFocalVector.normalized() ); |
I added this workaround in the above commit. Will leave this issue open to confirm that CT is happy. if ( firstFocalVector.equals( Vector2.ZERO ) ) {
// Prevent attempt to normalize a zero vector by ensuring that there is always a small non-zero x component.
// See https://github.com/phetsims/geometric-optics/issues/178
firstFocalVector.addXY( 1e-20, 0 );
} |
CT is happy. Closing. |
Reopening because there is a TODO marked for this issue. |
The text was updated successfully, but these errors were encountered: