Closed samreid closed 1 month ago
@zepumph and I took baby steps in the commit. Next we would like to:
MaterialEnumeration
. Use the strings everywhere. Promote nonCustomMaterialNames
to a new file, like a string union enumeration or keep in the same file Material.ts@samreid and I have a total mess with lots of good progress here in a patch. Upping the priority, and noting that this is a great direction, but exposed underlying problems with the "adjustableMaterial" paradigm for density mystery and B:B explore.
Bugs (that we know about)
next steps:
Slightly different patch from discussion with @zepumph
Almost ready for a commit, but need one more migration rule,
Here are the 5 spots that we do some mapping with material names/densities/material properties
ComboNumberControl for fluid selection MaterialControlNode for solid materials adjustableMaterial for density mystery B:B explore using MaterialControlNode + customDensityProperty/numberControl. CompareBlockSetModel.createMaterialProperty uses density to map to material, but there is no string enum complexity. Also it is derivedProperty, there is no bidirectional.
I'm wondering if we should put this issue on hold while investigating https://github.com/phetsims/density-buoyancy-common/issues/256
https://github.com/phetsims/density-buoyancy-common/issues/256 is looking promising! Catch you over there.
This has all been improved in that we only have Material now, and each instance is instrumented. Closing
Reopening because there is a TODO marked for this issue.
Still good to think about this one last todo, I'll decrease the priority until other material work is complete.
I wrote this patch which may be almost as good:
Also, maybe we don't need this at all since we never look up a Material based on a string name (other than tandems). In that case, we can just delete the list of MATERIALS and close the issue.
Excellent, thanks.
From some REVIEW comments in https://github.com/phetsims/density-buoyancy-common/issues/165
Note that material.identifier is being type asserted as
MaterialIdentifier | null
even though it is actuallyMaterialName | null
. What is the relationship betweenMaterialName
andMaterialIdentifier
? Do we need both? When would that type assertion fail?