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

lens/ProjectionScreen.js dependency common model code #173

Closed
zepumph opened this issue Aug 5, 2021 · 4 comments
Closed

lens/ProjectionScreen.js dependency common model code #173

zepumph opened this issue Aug 5, 2021 · 4 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Aug 5, 2021

While working on #154 I found myself pretty surprised as to how much ProjectorScreen is used inside of common model code. I initially read the implementation notes, including the gotcha, but I didn't realize how pervasive the link is. To me this is a spectrum of potential improvements that I would like to brainstorm. From easiest hardest.

  • It is a must that no code in the lens/ screen folder be used by common. This is more confusing that it is benefitial, even though those types are only part of the lens screen topic. At the very least, ProjectorScreen and its dependencies would move to common/model/. I hope this isn't the end of the conversation though.
  • It seems like LightRays is a pretty foundational piece of the model, it has knowledge about pretty much everything. Perhaps we could subtype LightRays to support projectorScreen and its implementation/feature only for the lens/ code. It looked like some amount of work, but not exhaustive.
  • Building on the aforementioned topic, and also pulling in thoughts from https://github.com/phetsims/geometric-optics/issues/168 (creating more general/generic optics types), why can't there be a base class of object that can interact with a LightRay. This interface can have the appropriate position, shape, absorbtion/refraction characteristics that it needs, without the hard coded assumptions about how it is used (like ProjectorScreen.js has). I didn't dive too deep into the code to figure out how we could do this, but I wanted to hear about @veillette's thoughts before doing too much research.

Let me know what you think!

@veillette
Copy link
Contributor

You are right that the dependencies of LightRays on ProjectorScreen is very hard corded as you point out. The position of the projector is used as a proxy to denote that the shape of the bisecting line has moved.

The projectorScreen role is solely to block rays and to send a signal that the rays have reached their destination.
In principle, this can be applied to any shape, so we could generalize it .

In the long run, it would be better to instantiate the projector screen in the lens model. At that point, it could inform the the lightRays that there exists a blocking shape through a method on lightRays.

@pixelzoom
Copy link
Contributor

ProjectorScreen.js is now ProjectionScreen.js.

@pixelzoom pixelzoom changed the title lens/ProjectorScreen.js dependency common model code lens/ProjectionScreen.js dependency common model code Oct 20, 2021
@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 27, 2021

At the very least, ProjectorScreen and its dependencies would move to common/model/

I disagree. ProjectorScreen (now ProjectionScreen) is relevant only to the Lens screen. The preferrable solution is to generalize LightRays so that it's not dependent on ProjectionScreen. (I think this is what @veillette was saying above in #173 (comment).) So that's the direction that I'm going to take.

@pixelzoom pixelzoom self-assigned this Oct 27, 2021
@pixelzoom
Copy link
Contributor

The dependency on ProjectorScreen has been removed from js/common/. The common model can now be provided with an optional "barrier", which is any object that implements interface Barrier. ProjectorScreen implements Barrier, is instantiated by LensModel, and is then passed to the superclass (common) model.

Closing.

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

3 participants