Remove NodeIO PhET-iO sub Properties

[zepumph]

Now that we have Properties in Node (see #490), (work stemming from discussion in https://github.com/phetsims/phet-io/issues/1642), we can likely replace the complicated pattern of having NodeIO defined PhET-iO instrumented Properties for instrumented Nodes.

Some of the specification that apply to this issue were outlined in #490 (comment) at last developer meeting:

• Eliminate NodeIO visibilityProperty, pickableProperty, opacityProperty ?
• Should pickableProperty and opacityProperty iO instrumentation be replaced by enabledProperty?
• Can visibleProperty be passed in via options? How about other Properties?

These are all good questions for this issue when @jonathanolson, @samreid, @chrisklus and I meet today to work on this issue.

[zepumph]

@jonathanolson, @samreid, @chrisklus, and I all worked on passing in visibleProperty to Node, and supporting it as an "instrumented sub Property" in the correct cases. This involved adding TinyForwardingProperty to make sure that Node.visibleProperty references would always be correct. We thought through multiple use cases, testing many of them, and they seemed to work in simple runs. We will want to create a better test suite though before too long.

There were a couple of loose ends that we want to try to fix before allowing opening up the visibleProperty API for mass consumption:

• Currently, if you instrument a Node it will create an instrumented sub Property. Then if you pass in an instrumented Property as the visibleProperty later, it will try to dispose the sub Property before swapping it out. PhET-iO API Validation does now allow disposal of stuff like this after sim startup. This may be a challenge to get around, and likely will need to be solved with dynamic element support.
• We set up logic that will add a LinkedElement to point to the passed in visibleProperty. We think it makes sense to add support to LinkedElement to "forward" client requests to its core type. That way you could still setValue on any LinkedElement. This may not be too important for Studio (since it shows the UI of the core element in a cut out), but it is likely that for invoke calls that we should support this. I'll create a new issue. (see LinkedElementIO should copy methods from its linked TypeIO tandem#171).
• There are some TODOs to try to fix some hackary in Joist for PhetMenu and PhetButton. Before we were using IO Types to remove the visiblePropertys from these, but I think that read-only should be plenty fine for making sure that clients don't make them invisible. The TODOs are linked to this issue.
• We still want to handle opacity and pickable Properties (see Uninstrument opacity and pickable Properties? #1047).
• @samreid mentioned seeing a lot of ownsXProperty booleans, and wonders if we shouldn't try to use this new TinyForwardingProperty pattern that was added for Node. (I think this deserves more discussion).
• We think some time should be taken to see if there are spots (especially in common code) where we can pass in a visibleProperty instead of using workaround-ish patterns that are there now before we had the visibleProperty option.
• We went over a lot of cases manually, but unit tests should be written for this new feature. Both phet and phet-io logic should be tested in both brands.

[chrisklus]

visibleProperty is broken in Studio on master. Marking blocks-publication and adding to the list in phetsims/ph-scale#164.

[chrisklus]

Fixed in the commit above. When creating the default instrumentedVisibleProperty, we were linking the not-yet-assigned class property instead of the Property variable. Tested with a few sims in Studio and phet-brand. Unmarking for blocks-publication.

[zepumph]

I see these points to move forward here:

• Wait on conclusion in Uninstrument opacity and pickable Properties? #1047 as to whether or not we need to support a default, phet-io instrumented,
• Support passing in Properties via options for anything else that is desired beyond visibleProperty
• Review changes, especially with usages of ForwardingProperty.

The second bullet feels like something we could determine without knowing the outcome of the first.

[zepumph]

I think that the visibilityProperty support code added above also needs a review here. I will do that as part of my review in #490. Some TODOs will be linked here though.

[zepumph]

• see (Move instrumented textProperty from IO Type to core type #1082) We should not forget to try to apply the same patter for TextIO.textProperty. Though I'm not sure how.

[zepumph]

After reviewing in #490, I would say that this issue is blocking publication until we feel confident that visibleProperty is being handled well in Node.js.

[zepumph]

Above is a large refactor of how setVisibleProperty works. It makes the linked element function (updateLinkedElementForVisibleProperty) much more graceful, and also simplifies Node.initializePhetioObject().

@samreid, since you left, the main change was that multiple Nodes in tests could not be disposed because they were created while expecting an assertion (via assert.throws()), and so had an inconsistent state that caused errors when calling dispose on them. I tried to document as best as I could. There are also a couple of new issues linked above coming from this work.

Next we need to work on mergePhetioComponentOptions and their usages for visibleProperty. Ideally we could remove this line soon:

Index: js/nodes/Node.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/nodes/Node.js	(revision 3c13c3f7348a650fc8488e3dc2b2d816eaeeac44)
+++ js/nodes/Node.js	(date 1602284248745)
@@ -5291,7 +5291,7 @@
           phetioReadOnly: this.phetioReadOnly, // pick the baseline value from the parent Node's baseline
           tandem: this.tandem.createTandem( 'visibleProperty' ),
           phetioDocumentation: 'Controls whether the Node will be visible (and interactive), see the NodeIO documentation for more details.'
-        }, this.phetioComponentOptions, this.phetioComponentOptions.visibleProperty, config.visiblePropertyOptions ) );
+        }, config.visiblePropertyOptions ) );
 
         this.setVisibleProperty( this.ownedPhetioVisibleProperty );
       }

@samreid does this belong in a separate issue?

Please review the above commits.

[samreid]

This unit test is failing:

    window.assert && assert.throws( () => {
      instrumented3.mutate( { visibleProperty: otherInstrumentedVisibleProperty } );
    }, 'cannot swap out one instrumented for another' );

I traced this and saw that when the LinkedElement is disposed, launched is false, so it is only being removed from the tandem buffer, not from the phetioEngine, so it is not triggering a validation error.

This has been a tricky case because Tandem.launched takes a different value based on what other tests have run previously.

[samreid]

The commits look great, I don't have any recommendations aside from the commits I made in NodeTests.js. I'm not confident to proceed on mergePhetioComponentOptions or phetioComponentOptions without further discussion--is our goal that we will replace all occurrences of phetioComponentOptions with passing in a custom instrumented BooleanProperty (which will be instrumented and linked instead of direct)? If so, I want to make sure that is an improvement before we proceed.

UPDATE: Or is the plan to replace cases like:

    options = merge( {

      // phet-io
      tandem: Tandem.REQUIRED,
      phetioComponentOptions: {

        // model controls visibility
        visibleProperty: {
          phetioReadOnly: true,
          phetioDocumentation: 'visibility is controlled by the model'
        }
      }
    }, options );

with

    options = merge( {

      // phet-io
      tandem: Tandem.REQUIRED,
      visiblePropertyOptions:{
        phetioReadOnly: true,
        phetioDocumentation: 'visibility is controlled by the model'
      }
    }, options );

?

[zepumph]

@samreid and I thought about a patch like this, but I think it is too constraining. For example if Panel wants its default visibleProperty to act a certain way, but we also support passing in a usage-specific visibleProperty. I think we should be tolerant of that:

Index: js/nodes/Node.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/nodes/Node.js	(revision e4673533f38500674f39a1a2d65bb78e306fa65a)
+++ js/nodes/Node.js	(date 1602697822420)
@@ -5297,6 +5297,10 @@
       }
       else {
 
+
+        // TODO: I'm worried this will be too constraining, what if a default button options want to add metadata https://github.com/phetsims/scenery/issues/1046
+        assert  && assert( !config.visiblePropertyOptions , 'Cannot specify visibleProperty and visiblePropertyOptions for default instrumented Node.visibleProperty.' );
+
         // Since we are just now instrumented, and linked elements can't be added to linked Elements until the PhetioObject
         // is instrumented, we need to retroactively link to whatever forwardingProperty may have been added before.
         this.updateLinkedElementForVisibleProperty( null, this.visibleProperty.forwardingProperty, false );

UPDATE: Here is an example of the EnabledComponent having strong assertions like this, and needing extra logic in Slider to get around providing both. I'm not sure this is the best way to do things, though it makes the mixin quite clear:
https://github.com/phetsims/sun/blob/2a4219ef0e10d931601a45420b9bf511993b6e0b/js/Slider.js#L128-L138

[zepumph]

RE: #1046 (comment), @samreid and I updated documentation, and are ok with not asserting.

[zepumph]

@samreid and I have completed all that we can see in this issue, and created side issues for next steps. We are now going to head over to #1047 to duplicate part/all of this pattern for opacity/pickable.