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

Instrument for PhET-iO #177

Closed
zepumph opened this issue Oct 4, 2019 · 6 comments
Closed

Instrument for PhET-iO #177

zepumph opened this issue Oct 4, 2019 · 6 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Oct 4, 2019

This sim is next on my list for PhET-iO instrumentation. Tagging @jbphet because he is the responsible dev.

My thoughts are to take a first pass before taking it to a design meeting.

@jbphet could you please note any current issues in the repo that should be taken care of before publishing off of master? Then you can unassign yourself.

@zepumph
Copy link
Member Author

zepumph commented Oct 9, 2019

Nevermind @jbphet, please see #178

@zepumph
Copy link
Member Author

zepumph commented Oct 10, 2019

Questions for a design meeting:

  • should VectorVisibilityProperties have a sub tandem, or should all its properties be directly on the view? In GQ we put them under 'viewProperties'; I went with 'visibilityProperties'.
    • rename to vectorVisibilityProperties
    • remove all the junk that doesn't apply to that specific screen.
  • There are custom-like number controls in some screen views, they are tentatively named "controlBox" and suffixed as such, perhaps we should discuss that.
  • Take a look at Keypad over in Instrument Keypad.js for PhET-iO scenery-phet#540.
    • instrument the KeypadLayer
    • knowing when the keypad is shown.
  • On lab screen, it would be good to look at how the altitude and drag coefficient boxes are set enabled. Currently the container that is set enabled is not instrumented, just its components.
    • lab screen custom controls, hide individual controls, like altitudeControl or dragCoefficientControl
    • no need to do anything with the parent node set enabled/disabled.
  • Rename launchVelocityProperty, Should launchVelocityProperty be renamed to initialSpeedProperty #191
  • Go through PhET-iO spec from client #190 to review/see if there are hickups. Likely that will be used as the base list to "feature" items.
  • Discuss *ControlBox terminology, because it isn't a type, just a node/container. In most places I renamed it to NumberControl when it was one.
    • In many cases, ControlBox should be "ReadOut" when just a line of text.
  • projectileMotion.introScreen.view.visibilityProperties.componentsForceVectorsOnProperty and same for "total" is instrumented on the introScreen even though there is no wiring for it, just velocity and acceleration.
    • see "remove all the junk that doesn't apply to that specific screen." above
  • lab screen air-resistance checkbox is center aligned right now.

@zepumph
Copy link
Member Author

zepumph commented Oct 10, 2019

After the above commits, studio and standalone are running when not validating tandems (but validating API).

@zepumph
Copy link
Member Author

zepumph commented Oct 11, 2019

As of now all required tandems have been filled in. The sim launches in studio and standalone while validating tandems. Studio can break pretty easily though because of incorrect serialization. Here is a list of next steps:

  • Instrument the harder model properties, like the list of projectiles
  • Instrument enumerations after conversions in separate issues
  • Handle some other code review issues that have popped up. This will be good to not put off so these don't pile up.
  • Get studio running when playing with the sim.
  • Add Groups for the projectiles and their view counterparts
  • Right now some ComboBox items are being created lazily, I think we likely want to refactor to create them eagerly.
  • Work on Keypad more if needed, likely not too much more before the design meeting, but it would be good to have a look in studio first, see Instrument Keypad.js for PhET-iO scenery-phet#540

@zepumph
Copy link
Member Author

zepumph commented Oct 18, 2019

I think we are ready for a PhET-iO meeting. The main agenda is #177 (comment), and it will be good to look through #190 too. Tagging @kathy-phet so she knows that I think it is time. In the interim I will work on the code review issues I've found (~10 issues I've opened through this work).

Tagging @samreid also because I did a bit different pattern for initial implementation. I dove straight into the PhET-iO instrumentation, and found code-review items as I went. When I thought that there was something that may need reworking, I left it alone, creating a side issue to handle a refactor+instrumentation separately. This has worked well for me, and I don't think that there were too many drawbacks to this more expedited approach. That said PM is simpler than many sims, and likely this wouldn't work for all sims. Furthermore, it is possible that I may THINK this was a good pattern, but future changes may occur that result in extra cost because of phet-io instrumentation already in there.

@zepumph
Copy link
Member Author

zepumph commented Nov 23, 2019

We have had a phet-io meeting where we went through #177 (comment), and much has been implemented. There are only a few issues left with the instrumentation before we are ready for phetioFeatured (#205). The main one is #208. I will go ahead and close this issue, and follow up in sub issues.

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