-
Notifications
You must be signed in to change notification settings - Fork 4
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
design PhET-iO API #58
Comments
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
…his node are not part of the API, #58 Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
I addressed most of the items that we identified in the 6/14/18 design meeting. Comments on the ones that I did not address:
This is a general problem, not specific to Hooke's Law, so I'm not going to attempt to address it in a sim-specific way. If this is something that is generally desirable, please create a general issue for discussion.
This is unrelated to the model. It's related to a view-specific Property,
This sounds like another general problem that shouldn't be addressed by Hooke's Law in a sim-specific way. If this is indeed generally desirable, please create a general issue for discussion.
@samreid said in Slack (and I agree) that we should revisit this. Is it really necessary? What is the use case? |
I had a Slack discussion with @arouinfar, and we decided that the following model Properties should not be instrumented.
Removing instrumentation for these Properties will result in significantly fewer messages in the data stream. |
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Completed #58 (comment) and logging has been improved. |
|
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Next step is to discuss #58 (comment) in a design meeting. |
Labeled with "block-publication". We're so close to a stable/general API that we should complete this before going creating a new release branch and going through the QA process. |
Going forward, it's my understanding that all PhET-iO APIs should be "thoughtful, sparse and stable". So I'm renaming this issue to "design and implement PhET-iO API". |
Unassigning until next PhET-iO design meeting. |
Unassigning until we resume work on Hooke's Law. |
8/16/18 design meeting, reviews open items in #58 (comment):
This is a general issue, will not be addressed here.
Wait until someone asks for it. The way to do this would be to instrument vectors, so that visibility of the value is accessible via the API. If we do add this, the client is responsible for hiding the "Values" checkbox. If they fail to do so, the user will still be able to change visibility of all values.
Wait until someone asks for this. This is more difficult, and involves significant HSlider work.
Wait until someone asks for it. This would add something to the API, not break it, and is relatively easy. |
I think we've addressed everything. I'll take another look through the issue before closing. |
Everything is indeed done. I'll leave this open so we can complete this Studio checklist item:
... but we can't proceed with that until we decide what to do about reentrant calls to Property |
Unassigning until this becomes unblocked. |
https://github.com/phetsims/phet-io/issues/1349 was unlabeled for blocks-sim-publication, reassigning to @pixelzoom to decide what's next. |
@samreid said:
Specifically, in https://github.com/phetsims/phet-io/issues/1349#issuecomment-421109776:
So we're going to move forward without addressing reentrant Properties. Next step is to therefore to complete this item from #58 (comment):
So essentially we need to do a review of the PhET-iO instrumentation. I'm not familiar with how PhET-iO instrumentation is reviewed - does PhET have a process or checklist for doing such a review? And I'm presuming that since I did the instrumentation, someone else should do the review. Assigning to @ariel-phet to prioritize and assign. |
On slack, @ariel-phet asked if I can respond to the preceding comment. Self assigning at elevated priority to put it on my list for near-future. |
The best course for review would be to assign to @zepumph and/or @samreid and optionally @chrisklus. The main steps will be to review the instrumentation primarily looking for (a) tandems that have names that don't match conventions (b) tandems that have structures or types that don't match conventions (c) extraneous or missing tandems, and (d) to look through the wrappers to make sure everything is as expected. We have a document about "How to Instrument a PhET Simulation for PhET-iO" here https://github.com/phetsims/phet-io/blob/master/doc/how-to-instrument-a-phet-simulation-for-phet-io.md but it is out of date and scheduled for revision in https://github.com/phetsims/phet-io/issues/1243. The document is not in a state where it would be helpful for @pixelzoom to use it as a pre-review checklist. Leaving assigned to @ariel-phet to see if there are questions or comments, and to delegate/prioritize/schedule. |
PhET-iO code review will be done in #68. Closing this issue since design and implementation have been completed. |
(CM edit: Whatever we do here must be compatible with client requirements in https://github.com/phetsims/phet-io-wrapper-hookes-law-energy/issues/2.)
6/14/18 design meeting:
Intro and Systems screens
Don't allow the io client to hide the spring.
(DONE: HookesLawSpringNode is no longer passed a tandem.)
Don't allow the io client to hide the arm.
(DONE: RoboticArmNode instruments its drag listener, but no longer passes tandem to supertype.)
Only display one scene or the other. Default to dual spring, not even have single spring.
(DONE: E.g.
seriesParallelProperty: 'series' and
sceneControl.visibleProperty: false`)Client should be able to turn model properties on and off
(DONE: All model Properties are instrumented. See RoboticArm and Spring.)
Client should be able to hide checkboxes.
(DONE: E.g.
valuesCheckbox.visibleProperty: false
)Don't allow client to change text
(DONE: The only text that is currently instrumented is coming from joist. Search for phetio IDs containing ".text")
Set a spring constant and hide that panel
(DONE: E.g.
springConstantProperty: 200
andspringConstantPanel.visibleProperty: false
)❌It looks odd to have a big empty stroked panel with only a few items in it. We need general layout engine or a hack like hiding the stroke/fill.
(Not supported common code, please create a general issue if this is desirable.)
Client should not be able to change the color of the spring
(DONE: HookesLawSpringNode is not passed a tandem.)
❌Can we allow showing value for some things but not others? @pixelzoom says we would need to redevelop that part of the model to make that possible
❌What about different units, like lbs/ft? Maybe the client could change strings?
(Will not address. This is contrary to "Don't allow client to change text" above. And changing strings should be done via translation query parameters.)
❌@ariel-phet asks: what about setting the slider range? Changing it from 200-600 to some subset? That would involve more work for HSlider, it does not currently have a mutable range.
(Not supported by HSlider or NumberControl, please create a general issue if this is desirable.)
Energy Screen
❌make it possible to hide the axis labels, so the students have to guess them
don't make it possible to show the equation on the chart (there is no such "equation" feature in this sim)
The client can get x,y pairs and make equivalent plots in the wrapper.
Documentation
phetioInstanceDocumentation
?(DONE: I added
phetioInstanceDocumentation
for anything that wasn't obvious)Refer clients to the model.md and maybe implementation-notes.md
Studio
The text was updated successfully, but these errors were encountered: