search

phetsims / greenhouse-effect

"Greenhouse Effect" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 4 forks source link

Missing dependencies for DerivedProperties #383

Closed pixelzoom closed 8 months ago

pixelzoom commented 8 months ago

Related to https://github.com/phetsims/axon/issues/441 ...

When run with ?strictAxonDependencies=true, greenhouse-effecf fails in multiple places with missing dependencies for DerivedProperties. All of these missing dependencies appear to be related to description strings.

pixelzoom commented 8 months ago

In https://github.com/phetsims/greenhouse-effect/commit/b0c79d745dad2cf2bcd1368213eff1b2fc9b8718, I fixed the cases that were straightforward by adding missing depenendencies.

In https://github.com/phetsims/greenhouse-effect/commit/789f6ae690b29838ff2e92c39781a86e903ef6be I fixed some additional cases that were straightforward. And I opted out of one difficult case by adding strictAxonDependencies: false. This case is difficult because string Property dependencies are buried deep inside functions that are call by the derivation of surfaceTemperaturePhraseProperty. @jbphet can decide whether to pursue this. (And yes, I realize that description strings are unlikely to be dynamic for quite awhile.)

Over to @jbphet for review and futher work, if he chooses to do so.

jbphet commented 8 months ago

I fixed the two remaining cases. In one of them I simply added all the string Properties that were potentially used in the creationof the description string to the dependences of the DerivedProperty. However, in the other case the list of dependencies was too long for the type definition of DerivedProperty. Apparently, DerivedProperty currently maxes out at 15 dependences, and I needed 19. I reached out on Slack for suggestions on how to best deal with this, and @samreid suggested I try Multilink.multilinkAny. This worked, and I committed it, but it feels like a bit of a loophole because it doesn't seem to check the dependencies the way DerivedProperty does.

This should work okay, and it will be a long time before description strings are dynamic, and we may be using an entirely different approach by then, so I'm okay with what we've got here. @samreid - can you have one last look at the commit above and make sure that I correctly understood your suggestion? If so, feel free to close this issue.

samreid commented 8 months ago

The changes look reasonable, but I believe I miscommunicated and there is opportunity for improvement. My message from slack is:

  /**
   * Create a DerivedProperty from any number of dependencies.  This is parallel to Multilink.multilinkAny
   */
  public static deriveAny

This is a static method in DerivedProperty. So I think you could continue using DerivedProperty here via DerivedProperty.deriveAny. Sorry for the miscommunication about "parallel to ..."

jbphet commented 8 months ago

My bad - I didn't read @samreid's Slack message carefully enough. I've updated the code to use deriveAny and it seems much better. I think this is good and can be closed.