Instrument using tandem

[jbphet]

We need to add instrumentation in support of together.js version, see https://github.com/phetsims/together/issues/32.

[samreid]

In the RGB screen, perceived color is a string instead of a color:

    this.addDerivedProperty( 'perceivedColor', [ 'perceivedRedIntensity', 'perceivedGreenIntensity', 'perceivedBlueIntensity' ],
      function( redIntensity, greenIntensity, blueIntensity ) {
        return 'rgb(' + [
            Math.floor( redIntensity * COLOR_SCALE_FACTOR ),
            Math.floor( greenIntensity * COLOR_SCALE_FACTOR ),
            Math.floor( blueIntensity * COLOR_SCALE_FACTOR ) ].join() + ')';
      }, {
        tandem: tandem.createTandem( 'perceivedColor' )
      } );

May make more sense to turn it into a scenery.Color before instrumenting.

[jbphet]

Version 1.1.0-together.1 was delivered to a customer who is using this in a demo system on May 6, 2015, and version 1.1.0-together.2 was delivered on May 26, 2015. I'm not putting the URL in this comment since this development is somewhat non-public at the moment.

[jbphet]

While testing the behavior, I noticed that the perceived color looks like this in the event stream:

"children": [
    {
      "messageIndex": 116,
      "eventType": "model",
      "togetherID": "colorVision.rgbBulbsScreen.perceivedColor",
      "componentType": "DerivedProperty",
      "event": "changed",
      "time": 1434053008578,
      "parameters": {
        "oldValue": {
          "listeners": [],
          "r": 0,
          "g": 0,
          "b": 39,
          "a": 1,
          "_css": "rgb(0,0,39)"
        },
        "newValue": {
          "listeners": [
            null,
            null,
            null,
            null
          ],
          "r": 0,
          "g": 1,
          "b": 39,
          "a": 1,
          "_css": "rgb(0,1,39)"
        }
      }
    }
  ]

It looks like the message is trying to include listeners and css values in the serialization, which is not correct. This issue is very similar to https://github.com/phetsims/beaker/issues/50.

[jbphet]

The problem with color serialization stems from the following portion of TogetherCommon.js in the DerivedProperty type variable (which is NOT the DerivedProperty defined in the axon repository):

      property.events.onStatic( 'startedCallbacksForChanged', function( newValue, oldValue ) {
        messageIndex = togetherEvents && togetherEvents.start( 'model', togetherID, 'changed', {
            oldValue: oldValue,
            newValue: newValue
          } );
      } );

The problem is that oldValue and newValue are being serialized automatically, so it attempts to include all references in the object. This code should probably check to see if the old and new values have a serialization method (i.e. toStateObject) and, if so, use it.

[samreid]

This pattern was already being used for ToggleButton:

      emitter.onStatic( 'startedCallbacksForToggled', function( oldValue, newValue ) {
        messageIndex = togetherEvents && togetherEvents.start( 'user', togetherID, 'toggled', {
            oldValue: valueType.toStateObject( oldValue ),
            newValue: valueType.toStateObject( newValue )
          } );
      } );

And is very different from the logic in Property wrapper:

      property.events.onStatic( 'startedCallbacksForChanged', function( newValue, oldValue ) {
        var oldValueForArch = null;
        var newValueForArch = null;
        if ( valueType.toStateObject ) {
          oldValueForArch = valueType.toStateObject( oldValue );
          newValueForArch = valueType.toStateObject( newValue );
        }
        else {
          oldValueForArch = oldValue;
          newValueForArch = newValue;
        }

        // If enabled, send a message to phet events.  Avoid as much work as possible if phet.togetherEvents is inactive.
        messageIndex = togetherEvents && togetherEvents.start( 'model', togetherID, 'changed', {
            oldValue: oldValueForArch,
            newValue: newValueForArch
          } );
      } );

I recommend (a) requiring all wrappers that appear as Property/DerivedProperty values must implement toDataObject and (b) switching DerivedProperty and Property to use the same pattern as ToggleButton. @jbphet sound OK?

[jbphet]

True, we should probably route the conversions through the type wrapper rather than just examining the instance and calling toStateObject on it if the method exists, since this will give us more flexibility and will allow us to implement handling of things like arrays. In a lot of cases, the wrapper type will just call toStateObject on the instance or simply return the instance itself (like for primitive types), but I guess we'll just need to live with this sort of boiler plate.

This seems like a general issue for together.js, so I'm going to create an issue there and reference this one.

[jbphet]

The together code has been modded as discussed above. The perceived color message looks much better, here's an example:

{
  "messageIndex": 167,
  "eventType": "model",
  "togetherID": "colorVision.rgbBulbsScreen.perceivedGreenIntensity",
  "componentType": "Property",
  "event": "changed",
  "time": 1434493886632,
  "parameters": {
    "oldValue": 36.983606557377016,
    "newValue": 36.196721311475414
  },
  "children": [
    {
      "messageIndex": 168,
      "eventType": "model",
      "togetherID": "colorVision.rgbBulbsScreen.perceivedColor",
      "componentType": "DerivedProperty",
      "event": "changed",
      "time": 1434493886632,
      "parameters": {
        "oldValue": {
          "r": 0,
          "g": 94,
          "b": 84,
          "a": 1
        },
        "newValue": {
          "r": 0,
          "g": 92,
          "b": 84,
          "a": 1
        }
      }
    }
  ]
}

[jbphet]

I believe this task is essentially done, though I'm sure changes will be made as our data collection framework evolves and as customers start to use the new capabilities. I'm comfortable closing this issue, and tracking any changes under new issues. I'll assign to @samreid to see if he's down with that and, if so, he can just close.

[samreid]

Sounds good @jbphet, thanks. Closing.