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

zoomLevelProperty should be discrete #337

Closed
arouinfar opened this issue Feb 28, 2022 · 5 comments
Closed

zoomLevelProperty should be discrete #337

arouinfar opened this issue Feb 28, 2022 · 5 comments

Comments

@arouinfar
Copy link
Contributor

For #237

The zoomLevelProperty is type NumberPropertyIO and accepts any value within the range 0-2. However, this range shouldn’t be treated continuously and it’s easy to break the sim with the current implementation. There are discrete zoom levels, each with its own rulers.

For an example of a discrete zoomLevelProperty see circuitConstructionKitDc.introScreen.model.zoomProperty.

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 28, 2022

Here's circuitConstructionKitDc.introScreen.model.zoomProperty:

screenshot_1574

The implementation of zoom in Geometric Optics bears no resemblance to CCK. CCK is an enumeration and has only 2 zoom levels. Geometric Optics is more general - zoom level is an integer, uses as an index into an array of scale values.

zoomLevelProperty is already 'Integer', see screenshot below. The fact that you're able to enter non-integers is a Studio problem with the new "value entry" UI, and I believe the iO team is working on adding validation.

@arouinfar let me know if I missed something here, or if you want to discuss reimplementing zoom level in Geometric Optics.

zoomLevelProperty

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 28, 2022

Also worth noting is that zoomLevelProperty is an integer NumberProperty because that's what is required by the common-code zoom controls. Doing what CCK is doing requires additonal code to map between integer and enumeration value, and the name zoomLevelProperty is then inconsistent with other sims. Since CCK is using a non-standard approach, perhaps it should consider renaming zoomLevelProperty to something non-standard that indicates that it's an enumertion.

@arouinfar
Copy link
Contributor Author

Thanks for the clarification @pixelzoom. I think my confusion is that the zoom magnification levels are 0.25x, 0.5x, and 1x, so I missed that zoomLevelProperty was an integer. I recommend adding phetioDocumentation that connects the value of zoomLevelProperty to the actual zoom magnification.

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 28, 2022

The actual scale value already exists as a DerivedProperty, so I've instrumented zoomScaleProperty. Note the validValues and documentation:

screenshot_1580

I improved the documentation for zoomIndexProperty:

screenshot_1581

What I do NOT want to do is put the actual scale values in the zoomIndexProperty documentation. That would be hard-coding something that is specified elsewhere in code, and could change in the future. So if (for some reason) the client wants to determine which zoomIndexProperty values correspond to which zoomScaleProperty values, they have to do a little work.

@arouinfar ready for review.

pixelzoom added a commit that referenced this issue Feb 28, 2022
pixelzoom added a commit that referenced this issue Feb 28, 2022
pixelzoom added a commit that referenced this issue Feb 28, 2022
pixelzoom added a commit that referenced this issue Feb 28, 2022
@pixelzoom pixelzoom assigned arouinfar and unassigned pixelzoom Feb 28, 2022
@arouinfar
Copy link
Contributor Author

Thanks @pixelzoom. I think this is a big improvement and really clarifies things.

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

2 participants