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

Review code for reuse #324

Closed pixelzoom closed 3 months ago

pixelzoom commented 11 months ago

For an up-coming new sim, PhET may want to reuse code from Projectile Motion, possibly creating a shared repository. To that end, @kathy-phet asked me to do a review of the projectile-motion code base.

pixelzoom commented 11 months ago

Evaluation of major PhET features

PhET-iO

PhET-iO instrumentation is incomplete. The Studio tree structure looks like a good start, but needs work. The sim definitely has problems in the Standard and State wrappers. For example, in the simplest case (firing 2 projectiles on the Intro screen) the sim is in an incorrect state when the Standard wrapper is launched from Studio. The indicators on the field that show where the projectiles landed are missing.

Alt Input

Not currently supported. No evidence of pdomOrder being set, and "supportsInteractiveDescription" is not set in package.json, which is what enables alt input.

UI Sound

Not currently supported. "supportsSound" is not set in package.json, which is what enables UI sound.

Dynamic Locale

Not currently supported. There are only 4 uses of StringProperty in the entire code base, which is what makes strings dynamic. Almost exclusively static strings passed to Text and RichText. No evidence that dynamic layout has been addressed.

TypeScript

Partially converted. 62% of the sim is TypeScript, with 3599 lines of JavaScript remaining. All of projectile-motions/js/common/ has been converted, so the remaining work is in screen-specific code.

pixelzoom commented 11 months ago

Evaluation of potential for code reuse

PM = Projectile Motion PM:DL = Projectile Motion: Data Lab

Vintage

The projectile-motion repo was created on 1/7/15, over 8 years ago. Version 1.0.0 was pubished on 8/23/2017. So this is some relatively old code. Any opportuinity for reuse is therefore likely to involve bringing old code up to new standards.

View

Since the view requirements of PM:DL will be different, I didn’t spend a lot of time evaluating the reusability of view components. We can certainly appropriate view components if they happen to be the same in both sims. But that can be evaluated “on the fly” as PM:DL is being implemented, not something to be done eagerly.

Model

Here is a list of the model classes in PM, and their suitabiltiy for reuse in PM:DL:

Conclusions

There is nothing obvious in the PM code base that provides a reason to eagerly create a shared repository. Creating such a repository will not reduce the time required to develop PM:DL. On the contrary, it will likely increase the time required, and destabilize PM.

My recommendation regarding code sharing is:

pixelzoom commented 11 months ago

@kathy-phet @matthew-blackman @AgustinVallejo @catherinecarter This is ready for your review. And I'm happy to discuss further. The last person assigned may close this issue.

catherinecarter commented 11 months ago

Thanks for doing such a thorough review of the PM code, @pixelzoom. It sounds like starting fresh is the best path.

I do wonder if keeping reusable components from PM:DL on our radar for future sims might be useful. I don't know what next sims will look like, but if there's structure built into the code from PM:DL that are challenging to implement but important to reuse, would it be valuable to create a new repo as we go? Or better to identify later?

pixelzoom commented 11 months ago

I do wonder if keeping reusable components from PM:DL on our radar for future sims might be useful. ...

I certainly try to make code general/reusable where I think they might be reusable in future sims. But there's an additional cost to do that, which must be balanced against milestones. Also, PhET generally does not create common code when it's only needed by one sim. The policy is that the 2nd sim to need something is responsible for migrating sim-specific code to common code, and generalizing it appropriately.

matthew-blackman commented 3 months ago

PDL and PSD have been published without reusing code from PM. This issue can be closed.