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

Add a flat mirror #227

Closed
kathy-phet opened this issue Oct 14, 2021 · 22 comments
Closed

Add a flat mirror #227

kathy-phet opened this issue Oct 14, 2021 · 22 comments

Comments

@kathy-phet
Copy link

In playing with the sim today, I was desiring to take the mirror flat. So adding this for consideration as we think about the final improvements after the prototype. Physics is basically just a virtual image behind the mirror and equidistant and same size as the object. Suggestion would be a third scene on mirror that was flat. Note - I haven't thought thru all the issues this suggestion may bring up. Just thinking about how flat mirrors are a pretty common goal/topic and what is in everyone's everyday life all the time.

@arouinfar
Copy link
Contributor

For phetsims/qa#723

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 14, 2021

I like this suggestion a lot. But I don't know what the implications would be. So far, I've been able to avoid changing the core model for the rays and images. Diving into that code will involve more time, more changes. I'm guessing this would add ~20-40 hours to my original estimate.

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 14, 2021

@kathy-phet said:

Suggestion would be a third scene on mirror that was flat.

I should point out that there are no "scenes" in this sim. Scenes have their own state, and that's not the case here for the different mirror (or lens) shapes. The radio buttons simply select the shape of the single mirror (or lens), they are not separate scenes.

We can certainly change this if you really want scenes, but it will be a significant lift.

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 19, 2021

Beyond the model support for a flat mirror, this is going to require significant changes througout the sim. There are multiple problems...

(1) There are currently "curve types" (concave, convex) and the code assumes that they are supported for both types of optic (lens, mirror). If we add a "flat" curve type that's supported only for mirror, that changes the assumption that all curve types are supported by all optics.

(2) The code assumes that the optic is either converging or diverging. A flat mirror is neither. So foundational code, like this function in Optic.js which is supposed to handle both lens and mirror, is not going to work correctly:

  isConverging( curve ) {
    return ( this.isConvex( curve ) && this.isLens() ) || ( this.isConcave( curve ) && this.isMirror() );
  }

@veillette
Copy link
Contributor

veillette commented Oct 21, 2021

In the way we implement the ray tracing and the images, a flat mirror shares many similarity with a converging mirror with an extremely long radius of curvature. Setting the max value of the radius of curvature to 100 000 cm, we can see that the model for the image behaves well and the ray tracing works correctly.
image

@kathy-phet
Copy link
Author

kathy-phet commented Oct 21, 2021 via email

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 21, 2021

That's good news that the model behaves like a flat mirror when given a large radius of curvature (ROC).

But it's one thing to poke a very large ROC into the code, and another thing to make it work in practice. Among the problems we would need to address are:

  • How to set ROC to this very large value when that Property is constrained to a smaller range.
  • How to handle a 3rd "curve type" which is neither concave or convex, and chase down all the isConvex and isConcave tests that appear in logic.
  • What to do with the radius of curvature control when the user selects "flat" for the mirror. (Hide it? Disable it with a bogus value showing?...)
  • How to deal with the fact that focal length for a flat mirror should be infinity, but will not be infinity if we're faking it with a large ROC. This will be very apparent in PhET-iO.

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 21, 2021

10/21/2021 design meeting: @arouinfar @kathy-phet @ariel-phet

  • Hollywood this, using a large ROC for a concave (converging) mirror, as @veillette demonstrated in Add a flat mirror #227 (comment).
  • Add a 3rd radio button for "flat" mirror shape
  • Hide ROC control when "flat" mirror is selected
  • Bogus values (ROC, focal length) in PhET-iO are OK. They can be explained in PhET-iO client guide.

@pixelzoom pixelzoom changed the title Consider a flat mirror Add a flat mirror Oct 21, 2021
@pixelzoom pixelzoom assigned pixelzoom and unassigned arouinfar Oct 21, 2021
@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 20, 2021

For a flat mirror, change the Diameter label to "Height" in the Basics version of the sim.

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 21, 2022

In #255, we added an option to replace ROC+IOR controls with a Focal Length control. I'm wondering what we should do with the Focal Length control when 'flat' mirror is selected. Hide the control? Change it to a read-out that shows infinity? Something else?

Also wondering if we should replace the ROC control with a read-out that shows infinity, instead of hiding it totally.

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 21, 2022

Discussed with @arouinfar.

  • When 'flat' mirror is chosen, hide ROC and Focal Length controls. There's not much value in showing the (fixed) infinity value, and it would be a significant amount of work. OK that the Mirror control panel will resize.

  • Order of radio buttons for mirror shape is [ ) ] [ ( ] [ | ].

@arouinfar
Copy link
Contributor

The flat mirror looks great. It's a bit odd for it to be off-center, but for consistency, I know that it needs to stay in place. However, for Basics, I think we should center the flat mirror in the play area since it's the only type of mirror.

If you inspect focalLengthProperty and radiusOfCurvatureProperty in Studio, you'll see that they are both "POSITIVE_INFINITY"

This looks good in Studio and I think it's best to use the real values in the model. Looks like twiceFocalLengthProperty, right2FProperty and left2FProperty are all using the very large view values, though. Not sure if this is an oversight or expected.

We should also revisit the initial position of the optical object in the Mirror screen. If you go to the Mirror screen and select 'flat' mirror before moving the object, the optical image is outside the visible bounds of the screen.

Agreed. The tricky part will be keeping the image position on-screen for all 3 types of mirrors. Shifting the object closer to the mirror will ensure the image of the flat mirror is starts on-screen, but will push the image of the concave mirror off-screen. An initial position of { "x": -130, "y": 72.5 } looks pretty good to me if we reduce also the initial ROC to 180 cm.

image

image

@pixelzoom
Copy link
Contributor

This looks good in Studio and I think it's best to use the real values in the model. Looks like twiceFocalLengthProperty, right2FProperty and left2FProperty are all using the very large view values, though. Not sure if this is an oversight or expected.

I can certainly pull that off. But I'll need to add 5 Properties. For any Property that can have value Infinity, I need to add a derived (and uninstrumented) "finite" Property. Here are the 5 Properties that can have value Infinity, and the 5 "finite" Properties that I'll need to add:

  • leftFocalPointProperty, finiteLeftFocalPointProperty
  • rightFocalPointProperty, finiteRightFocalPointProperty
  • this.twiceFocalLengthProperty, this.finiteTwiceFocalLengthProperty
  • this.left2FProperty, this.finiteLeft2FProperty
  • this.right2FProperty, this.finiteRight2FProperty

@pixelzoom
Copy link
Contributor

An initial position of { "x": -130, "y": 72.5 } looks pretty good to me if we reduce also the initial ROC to 180 cm.

That seems to work for the framed objects. What initial positions do you want for the 2 arrows?

pixelzoom added a commit that referenced this issue Feb 22, 2022
@pixelzoom
Copy link
Contributor

@arouinfar and I met on Zoom to adjust the initial positions of objects in the Mirror screen, as well as the initial ROC and focal length. Those changes are in the above commit.

As for the issue of showing physically-correct values as discussed in #227 (comment) ... This is a slippery slope. There are at least 7 such Propertes: focalLengthProperty, radiusOfCurvatureProperty, leftFocalPointProperty, rightFocalPointProperty, twiceFocalLengthProperty, left2FProperty, right2FProperty.

(1) Show physically-correct values for all Properties. This is more development work, makes the code more difficult to understand, and requires documentation warning the client that their programs need to handle Infinity.

(2) Show physically-correct values for focalLengthProperty and radiusOfCurvatureProperty, finite (Hollywood) values for the others. This is less development work than (1), and is inconsistent: it still makes the code difficult to understand, and still requires documentation warning the client that their programs need to handle Infinity.

(3) Show finite (Hollywood) values for all Properties. This is the least amount of development work, and the code is easiest to understand. Client documentation is still needed, but telling them to interpret very-large values as "infinity".

We're currently doing (2), which I think is the least desirable option. I'm not going to do anything further about this until @arouinfar and @kathy-phet have discussed.

@arouinfar
Copy link
Contributor

@kathy-phet can you take a look at this issue when looking over Geometric Optics?

@arouinfar
Copy link
Contributor

@pixelzoom I'm leaning towards option (3). I just don't think (1) is worth the hassle and (2) is increasingly strange, especially now that we expose the magnificationProperty in the tree. For the flat mirror, the magnification is not exactly 1. This is not unexpected given the approach of approximating the flat mirror with a large ROC.

@arouinfar arouinfar removed their assignment Feb 28, 2022
@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 28, 2022

@arouinfar I agree, (3) seems best. (1) is a slippery slope.

Would you like me to implement (3), or are we still waiting for input from @kathy-phet?

@arouinfar
Copy link
Contributor

Let's proceed with (3) @pixelzoom.

@pixelzoom
Copy link
Contributor

Done in the above commit. @arouinfar please review in master.

@arouinfar
Copy link
Contributor

Looks good in master, thanks @pixelzoom

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

4 participants