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

Decouple "framed object" and "light source". #217

Closed
pixelzoom opened this issue Oct 5, 2021 · 3 comments
Closed

Decouple "framed object" and "light source". #217

pixelzoom opened this issue Oct 5, 2021 · 3 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 5, 2021

Framed objects and light sources are currently combined into one "representation", differentiated by the isObject field of Representation. This "combining" is carried through the implementation of the model and the view.

Disadvantages:

  • A bunch of isObject tests are required througout the code, with different code paths for Object vs Light Source. This is not very object-oriented, complicates the model and view, and makes it difficult to address issues like Second light source can be dragged to the right of the lens. #58.

  • SourceObject and SourceObjectNode handle the model and view respectively for Objects and the first light source.

  • SecondSource and SecondSourceNode handle the model and view respectively for the 2nd point of interest on the Object, and the second light source. This makes it very difficult for the first and second light sources to share implementation and behavior, for example Second light source can be dragged to the right of the lens. #58.

  • It's difficult to add additional representations. For example, it will be difficult (impossible?) to support the classic "arrow" representation for the Object , like this example from Wikipedia:

screenshot_1315

In this representation, the arrow's tail is anchored to the optical axis. The arrow can be resized and translated along the optical axis. This representation has come up several time in design meetings.

  • This approach is going to result in much confusion when it is exposed via the PhET-iO API and Studio.

For the longterm, it would be best to decouple "object" and "light source", in a way that supports other Object representations like "arrow", and allows light sources (first and second) to share the same implementation. This will be a big task, and will be destabilizing.

@pixelzoom pixelzoom self-assigned this Oct 5, 2021
@pixelzoom pixelzoom removed their assignment Oct 7, 2021
@pixelzoom pixelzoom self-assigned this Oct 21, 2021
pixelzoom added a commit that referenced this issue Jan 31, 2022
@pixelzoom pixelzoom changed the title Decouple "object" and "light source". Decouple "framed object" and "light source". Feb 2, 2022
@pixelzoom
Copy link
Contributor Author

In the above commits, I did a bit of cleanup and made some simpler changes before diving into the major changes.

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

pixelzoom commented Feb 7, 2022

Major changes in the above commit. Framed objects and light sources are now mostly decoupled. And I've introduced the "scenes" that I discussed with @arouinfar last week. This work is not complete, but this felt like a good check point to commit.

NOTE that a few features are temporarily out-of-service:

  • rulers and toolbox are hidden
  • Labels checkbox does not work
  • Show/Hide toggle button does not work correctly
  • Guides do not appear for framed objects

Here's my high-level TODO list:

  • Complete implementation of LightSource
  • Factor out LightSourceNode from SourceObjectNode
  • Rename SourceObject to FramedObject, SourceObjectNode to FramedObjectNode
  • Move SecondPoint into FramedObject
  • Replace Representation and representationProperty
  • Restore rulers and toolbox
  • Restore Labels feature
  • Restore Show/Hide toggle button feature
  • Restore Guides feature
  • Factor out duplication in scenes (model & view)

@pixelzoom
Copy link
Contributor Author

This work is done. The bits of duplication will be addressed after the Arrows scene is added in #228.

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

1 participant