[Density - Mystery Screen] MaterialProperty has "optionalTandem" valid values

[arouinfar]

Related to #270 and phetsims/buoyancy#51

I reviewed the valid values of materialProperty for all blocks/objects in the Density/Buoyancy suite, and discovered that Density's Mystery screen has some extraneous options. There are three "optionalTandem" options under valid values, which should be removed.

Assigning to @AgustinVallejo and @samreid.

[samreid]

I'll take a look. Solo-assigning for now

[samreid]

From this patch, I can see the optionalTandems are copper, steel and platinum:

Subject: [PATCH] Add documentation and TODOs, see https://github.com/phetsims/density-buoyancy-common/issues/123
---
Index: js/common/model/Mass.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Mass.ts b/js/common/model/Mass.ts
--- a/js/common/model/Mass.ts	(revision 2a6d113f09c3667ce4c9ef102c8fdd5cf4cd0e81)
+++ b/js/common/model/Mass.ts	(date 1722632737249)
@@ -247,10 +247,16 @@
     }, options.customMaterialOptions ) );
 
     const initialMaterial = options.material === 'CUSTOM' ? customSolidMaterial : options.material;
+    const myMaterials = options.availableMassMaterials.map( x => x === 'CUSTOM' ? customSolidMaterial : x );
     this.materialProperty = new MaterialProperty( initialMaterial, customSolidMaterial,
-      options.availableMassMaterials.map( x => x === 'CUSTOM' ? customSolidMaterial : x ),
+      myMaterials,
       options.materialPropertyOptions as MaterialPropertyOptions );
 
+    myMaterials.forEach( material => {
+      console.log( material.nameProperty.value );
+      console.log( material.phetioID );
+    } );
+
     // There are complicated order dependencies in how DynamicProperties unlink to old values and link to new ones. See https://github.com/phetsims/buoyancy/issues/67
     // Note: Just NOTIFY didn't work, neither did internalVisibleProperty.addPhetioStateDependencies()
     if ( this.materialProperty.isPhetioInstrumented() ) {

I can also see that those 3 materials do exist in density 1.1:

Therefore, it seems we should instrument those and keep them, unless we decided to purposefully remove them.

[samreid]

I did not see mention of purposefully removing copper, platinum, steel in #282 where they were removed. So I restored them in the commit.

Next I observed the same problem for gravity (some optionalTandems in density), and I will work on that next (probably removing them in that case).

[arouinfar]

I did not see mention of purposefully removing copper, platinum, steel in #282 where they were removed. So I restored them in the commit.

@samreid I didn't list all of the materials, but this falls under the last point, "Materials should only appear in the trees of relevant sims." Copper, platinum, and steel are not user-selectable materials in Density.

Next I observed the same problem for gravity (some optionalTandems in density), and I will work on that next (probably removing them in that case).

Good catch! Sounds good.

[arouinfar]

I somehow missed @samreid's first comment in #307 (comment). I don't know why platinum, copper, and steel were valid values in Density 1.1. That said, I don't think it's worth the effort to remove them in 1.2, as their presence is benign.

[samreid]

Sounds good, thanks. Next, I removed the uninstrumented gravity values on density and buoyancy-basics via a filter. It was easy to find them by searching for optionalTandem in the API files.

I think this issue is ready for review.

[arouinfar]

Thanks @samreid looks good on main, closing.