complete the "Real Molecules" screen

[pixelzoom]

Because of the time involved in implementing a 3D molecule viewer (#15), the decision was made to publish Molecule Polarity without the Real Molecules screen (#27).

When the Real Molecules screen is added in a future version, here's what needs to be done:

• Implement a 3D molecule viewer, implement 3D molecule viewer for "Real Molecules" screen #15
• Address TODO issues in the code that reference issue implement 3D molecule viewer for "Real Molecules" screen #15
• Code review for any new code that is required to complete the screen
• Add description of the 3D model to model.md
• Add description of the 3D implementation to implementation-notes.md

[oliver-phet]

From phethelp:

I appreciate all the work that has been done to maintain PhET invaluable simulations through converting them to HTML5 format. Without taking too much of your time, I'm writing to request that someone work to adapt the "Real Molecules" function of the Molecule Polarity simulation to HTML5. Its so important for students to see the effect of polarity on larger, organic molecules with multiple dipoles to understand the concepts of solubility.

[pixelzoom]

Status for 1/6/2022 quarterly-planning meeting:

The status of this is unchanged since the last meeting:

• Basic 3D common-code exists, but nothing specifically related to molecules or this sim.
• We need a meeting to decide who is doing what, and how to move forward.
• The ball is in @kathy-phet's court.

[samreid]

More strict assertions introduced in phetsims/sun#920 make a workaround here more difficult. The simulation fails CTQ with:

Error in molecule-polarity: Error: Assertion failed: Error: Assertion failed: Property should be instrumented if AquaRadioButtonGroup is instrumented
    at window.assertions.assertFunction (http://localhost:40339/assert/js/assert.js:45:21)
    at http://localhost:40339/chipper/dist/js/joist/js/simLauncher.js:24:31
/data/share/phet/continuous-testing/ct-quick/chipper/js/phet-io/generatePhetioMacroAPI.ts:110
        throw new Error( workerResult.reason );

To resolve CTQ for now, I will remove the workaround in

molecule-polarity/js/common/model/MPPreferences.ts

Lines 25 to 37 in f376310

	surfaceColorProperty: new StringUnionProperty<SurfaceColor>(
	MPQueryParameters.surfaceColor as SurfaceColor, {
	validValues: SurfaceColorValues,
	//TODO see https://github.com/phetsims/molecule-polarity/issues/32
	// Until the 'Real Molecules' screen is fully implemented, opt out of PhET-iO instrumentation.
	// In the meantime, support testing via the realMolecules query parameter.
	tandem: ( MPQueryParameters.realMolecules ) ?
	Tandem.PREFERENCES.createTandem( 'surfaceColorProperty' ) :
	Tandem.OPT_OUT.createTandem( 'surfaceColorProperty' ),
	phetioDocumentation: 'color scheme for the Electrostatic Potential surface in the Real Molecules screen'
	} )

but this issue will likely need attention before next publication.

[pixelzoom]

@samreid I have concerns about e04b6bd and https://github.com/phetsims/phet-io-sim-specific/commit/762977427724d351a5d9900f8f241c73bffa5a8e. I don't think we want to be adding PhET-iO elements related to the Real Molecules screen, as it appears you've done for surfaceColorProperty. Until then, the tandem for surfaceColorProperty should be Tandem.OPT_OUT.createTandem( 'surfaceColorProperty' ).

[pixelzoom]

Here's a patch for what I think is a preferrable way to address the new constraints introduced for AquaRadioButtonGroup in phetsims/sun#920. It does change the published API in that it uninstruments preferencesPanels.simulationPreferencesPanel.simPreferences.surfaceColorControl -- which arguably should not have been instrumented in the published version, because it's only relevant for the Real Molecules screen.

@samreid do you see any problem with this?

patch

Subject: [PATCH] revert to "Spectrometer" for accordion box title
---
Index: js/common/model/MPPreferences.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/MPPreferences.ts b/js/common/model/MPPreferences.ts
--- a/js/common/model/MPPreferences.ts	(revision c3d2361c28db53cd98edba385e7ce59da17f0c58)
+++ b/js/common/model/MPPreferences.ts	(date 1735618395876)
@@ -27,8 +27,9 @@
     MPQueryParameters.surfaceColor as SurfaceColor, {
       validValues: SurfaceColorValues,
 
-      // TODO: It would be preferable to uninstrument this when running without the Real Molecules screen, which is not yet phet-io instrumented, see https://github.com/phetsims/molecule-polarity/issues/32
-      tandem: Tandem.PREFERENCES.createTandem( 'surfaceColorProperty' ),
+      //TODO When Real Molecules screen is eventually implemented, make this tandem change. See https://github.com/phetsims/molecule-polarity/issues/32
+      // tandem: Tandem.PREFERENCES.createTandem( 'surfaceColorProperty' ),
+      tandem: Tandem.OPT_OUT,
       phetioDocumentation: 'color scheme for the Electrostatic Potential surface in the Real Molecules screen',
       phetioFeatured: true
     } )
Index: js/common/view/MPPreferencesNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/MPPreferencesNode.ts b/js/common/view/MPPreferencesNode.ts
--- a/js/common/view/MPPreferencesNode.ts	(revision c3d2361c28db53cd98edba385e7ce59da17f0c58)
+++ b/js/common/view/MPPreferencesNode.ts	(date 1735618395869)
@@ -13,6 +13,7 @@
 import optionize, { EmptySelfOptions } from '../../../../phet-core/js/optionize.js';
 import PickRequired from '../../../../phet-core/js/types/PickRequired.js';
 import { VBox, VBoxOptions } from '../../../../scenery/js/imports.js';
+import Tandem from '../../../../tandem/js/Tandem.js';
 import moleculePolarity from '../../moleculePolarity.js';
 import MPPreferences from '../model/MPPreferences.js';
 import MPQueryParameters from '../MPQueryParameters.js';
@@ -41,7 +42,9 @@
     } );
 
     const surfaceColorControl = new SurfaceColorControl( MPPreferences.surfaceColorProperty, {
-      tandem: options.tandem.createTandem( 'surfaceColorControl' )
+      //TODO When Real Molecules screen is eventually implemented, make this tandem change. See https://github.com/phetsims/molecule-polarity/issues/32
+      // tandem: options.tandem.createTandem( 'surfaceColorControl' )
+      tandem: Tandem.OPT_OUT
     } );
 
     //TODO https://github.com/phetsims/molecule-polarity/issues/32

[samreid]

Yes, that seems sensible to me. Will we need to add a corresponding DeleteUninstrumentedPhetioElement in the migration processors?

[pixelzoom]

Yes, we'll need a DeleteUninstrumentedPhetioElement for surfaceColorControl.

[samreid]

surfaceColorControl was phetioState:false in 1.3.9, so so migration processors are not necessary. I reviewed, tested, regenerated-phet-io-api, committed, and pushed.

@pixelzoom anything else here?

[samreid]

It does seem we need a migration processor after all, not due to surfaceColorControl itself but to its children. I'll commit momentarily.

[pixelzoom]

👍🏻 Closing.

[phet-dev]

Reopening because there is a TODO marked for this issue.

[pixelzoom]

Oops, I should not have closed this issue -- which is about completing the Real Molecules screen, and not about addressing changes related to phetsims/sun#920. In retrospect, it would have been preferrable to create a new issue to address those changes.