phetsims / projectile-motion

"Projectile Motion" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
15 stars 13 forks source link

How to instrument vectorVisibilityProperties #224

Closed zepumph closed 3 years ago

zepumph commented 4 years ago

From #219, @arouinfar had the review comment like:

It's not really clear why projectileMotion.introScreen.view.vectorVisibilityProperties is in the view. This seems analogous to gravityAndOrbits.modelScreen.model.showGravityForceProperty (both toggle the visibility of a vector in the play area), which is in the model.

I see. I also see graphingQuadratics.exploreScreen.view.viewProperties, which makes me feel like it is a bit better to just go with how the sim has implemented these. I can see them fitting in either, if there is a strong preference, then we can probably refactor, but to be honest I would prefer to see them in the view. They have nothing to do with the projectiles' model, and instead only with how information is displayed (hence the view). This is not to say that the GAO instrumentation is incorrect, but perhaps the decision in GAO would be different if there were multiple (4 in PM and 7 in GQ).

What do you think @arouinfar @kathy-phet?

zepumph commented 4 years ago

While we are at it over here, we should discuss the name too, from this comment in #219:

The "VisibilityProperties" ending to projectileMotion.introScreen.view.vectorVisibilityProperties seems pretty close to visibleProperty, which is a bit of a loaded meaning. Perhaps we should name it something different? @kathy-phet can't remember what we decided for this.

I personally think that these Properties are in fact controlling the visibility of items, and items that aren't instrumented, and so don't have their own visibleProperty (which I think is the right thing to do). Thus to me the name makes sense. What might you prefer instead?

arouinfar commented 4 years ago

@zepumph thanks for the explanation! I can see how the vectorVisibilityProperties makes more sense in the view. Since our initial review we've also seen other sims with similar view properties. I'm happy to leave things the way they are. I'll leave this open so I can double check with @kathy-phet the next time we review this sim.

arouinfar commented 3 years ago

Reviewed in #244

@zepumph several sims collect view properties in a container called viewProperties, and we'd like to do the same here. Can you rename vectorVisbilityProperties to viewProperties? This would apply to all relevant screens.

zepumph commented 3 years ago

This issue should be reviewed at the next time Projectile Motion is worked on.

zepumph commented 3 years ago

Alright, classes and tandems have been renamed to viewProperties. We now have:

projectileMotion.introScreen.view.viewProperties. projectileMotion.vectorsScreen.view.viewProperties. projectileMotion.dragScreen.view.viewProperties.

Anything else here @arouinfar?

arouinfar commented 3 years ago

Looks good, thanks!