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

Strange order dependency between zoomTransformProperty and zoomScaleProperty #295

Closed
pixelzoom opened this issue Jan 18, 2022 · 4 comments
Closed
Assignees
Labels
type:bug Something isn't working

Comments

@pixelzoom
Copy link
Contributor

assert.js:25 Uncaught Error: Assertion failed: space for units label is negative or zero
    at window.assertions.assertFunction (assert.js:25:13)
    at new RulerNode (RulerNode.js? [sm]:147:21)
    at createRulerNode (GORulerNode.ts:340:10)
    at GORulerNode.ts:136:22
    at TinyProperty.emit (TinyEmitter.ts:93:9)
    at DerivedProperty._notifyListeners (Property.ts:289:23)
    at DerivedProperty.set (Property.ts:223:14)
    at DerivedProperty.getDerivedPropertyListener (DerivedProperty.ts:131:13)
    at TinyProperty.emit (TinyEmitter.ts:93:9)
    at NumberProperty._notifyListeners (Property.ts:289:23)

To reproduce manually:

  1. Run the sim with ?ea
  2. Go to Lens screen
  3. Pull a ruler out of toolbox
  4. Press zoom-out (-) button
  5. Press zoom-in (+) button. The assertion will fail.
@pixelzoom pixelzoom added the type:bug Something isn't working label Jan 18, 2022
@pixelzoom pixelzoom self-assigned this Jan 18, 2022
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 18, 2022

Scaling of rulers got messed up sometime since 1.1.0-dev.14 was published on 1/12/2022 @ 12:51. Their view length should not change when zooming in/out, only the scale on the ruler should change. But the view length of the ruler is now changing.

For example, here's the horizontal ruler initially:

screenshot_1527

... and after pressing the zoom-out button once:

screenshot_1528

@pixelzoom
Copy link
Contributor Author

Bisecting, I isolated this bug to 9802b4f.

pixelzoom added a commit that referenced this issue Jan 18, 2022
@pixelzoom
Copy link
Contributor Author

As noted in the above commit:

    //TODO There's a strange order dependency between zoomTransformProperty and zoomScaleProperty. If
    // zoomScaleProperty is defined first, then zoomTransformProperty is incorrect, and rulers will be
    // messed up as in https://github.com/phetsims/geometric-optics/issues/295. This has something to
    // do with the fact both Properties call getAbsoluteZoomScale.

So there's more work to do here. And this is another good reason to reimplement model-view transforms in #277.

@pixelzoom pixelzoom changed the title Assertion failed: space for units label is negative or zero Strange order dependency between zoomTransformProperty and zoomScaleProperty Jan 18, 2022
@pixelzoom
Copy link
Contributor Author

Addressed in #277, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant