phetsims / gravity-and-orbits

"Gravity And Orbits" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
12 stars 6 forks source link

Recording and playback wrappers do not work #387

Closed KatieWoe closed 3 years ago

KatieWoe commented 3 years ago

Device Dell OS Win 10 Browser Firefox Problem Description For https://github.com/phetsims/QA/issues/660 Was not fixed by fix in https://github.com/phetsims/phet-io/issues/1790. The playback wrapper does not seem to recognize the device/browser the recording was made on. When trying to play, the recording never loads and the console throws an error. Visuals

recordingbad nouseragent
samreid commented 3 years ago

Great discovery, I was able to reproduce the problem. I tracked it back to our changes in SimInfo, and made a correction. It's now working correctly on my side. Can you please pull all and test again?

KatieWoe commented 3 years ago

I still seem to be having this problem

samreid commented 3 years ago

@KatieWoe and I worked together, did a fresh pull and build and confirmed it is working, closing.

zepumph commented 3 years ago

A couple of review items here as I was looking at https://github.com/phetsims/gravity-and-orbits/issues/386

https://github.com/phetsims/joist/blob/d95fa578c2341f20325fc5c69bd88cadfb93c558/js/SimInfo.js#L134

This should call on simInfo.info, and I think would just return undefined right now.

https://github.com/phetsims/joist/blob/d95fa578c2341f20325fc5c69bd88cadfb93c558/js/SimInfo.js#L129-L133

Instead of duplicating these values, instead can't we just use simInfo.info.[KEY]?

I like what I see in the commit in general, but I'm wondering why we would only add into state the items that were needed for this playback. Here is the group a data that was added in this issue:

    // Parts that are omitted in API generation
    randomSeed: NullableIO( NumberIO ),
    url: NullableIO( StringIO ),
    userAgent: NullableIO( StringIO ),
    window: NullableIO( StringIO ),
    referrer: NullableIO( StringIO ),
    flags: NullableIO( StringIO )
  }
} );

Here is the putInfo that is not added to state ever:


// globals
this.putInfo( 'language', window.navigator.language );
this.putInfo( 'checkIE11StencilSupport', Utils.checkIE11StencilSupport() );
this.putInfo( 'isWebGLSupported', Utils.isWebGLSupported );
this.putInfo( 'pixelRatio', `${window.devicePixelRatio || 1}/${backingStorePixelRatio}` );

Why wouldn't we just add all of this also to be complete? Then everything in simInfo is also in state. These would also be hidden behind Tandem.API_GENERATION.

Here is a patch that includes all of the above review comments, @samreid if you agree would you please review or apply it.

I tested this by:

```diff Index: joist/js/SimInfo.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/SimInfo.js b/joist/js/SimInfo.js --- a/joist/js/SimInfo.js (revision 405a7d3b680eb3eb3fc55f198e047d8d1c2244ef) +++ b/joist/js/SimInfo.js (date 1625150289106) @@ -12,6 +12,7 @@ import PhetioObject from '../../tandem/js/PhetioObject.js'; import Tandem from '../../tandem/js/Tandem.js'; import ArrayIO from '../../tandem/js/types/ArrayIO.js'; +import BooleanIO from '../../tandem/js/types/BooleanIO.js'; import IOType from '../../tandem/js/types/IOType.js'; import NullableIO from '../../tandem/js/types/NullableIO.js'; import NumberIO from '../../tandem/js/types/NumberIO.js'; @@ -122,16 +123,20 @@ repoName: simInfo.info.repoName, screenPropertyValue: simInfo.info.screenPropertyValue, - wrapperMetadata: simInfo.info.wrapperMetadata, dataStreamVersion: simInfo.info.dataStreamVersion, phetioCommandProcessorProtocol: simInfo.info.phetioCommandProcessorProtocol, - randomSeed: Tandem.API_GENERATION ? null : window.phet.chipper.queryParameters.randomSeed, - url: Tandem.API_GENERATION ? null : window.location.href, - userAgent: Tandem.API_GENERATION ? null : window.navigator.userAgent, - window: Tandem.API_GENERATION ? null : `${window.innerWidth}x${window.innerHeight}`, - referrer: Tandem.API_GENERATION ? null : document.referrer, - flags: Tandem.API_GENERATION ? null : simInfo.flags || null + wrapperMetadata: Tandem.API_GENERATION ? null : simInfo.info.wrapperMetadata, + randomSeed: Tandem.API_GENERATION ? null : simInfo.info.randomSeed, + url: Tandem.API_GENERATION ? null : simInfo.info.url, + userAgent: Tandem.API_GENERATION ? null : simInfo.info.userAgent, + window: Tandem.API_GENERATION ? null : simInfo.info.window, + referrer: Tandem.API_GENERATION ? null : simInfo.info.referrer, + language: Tandem.API_GENERATION ? null : simInfo.info.language, + pixelRatio: Tandem.API_GENERATION ? null : simInfo.info.pixelRatio, + isWebGLSupported: Tandem.API_GENERATION ? null : simInfo.info.isWebGLSupported, + checkIE11StencilSupport: Tandem.API_GENERATION ? null : simInfo.info.checkIE11StencilSupport, + flags: Tandem.API_GENERATION ? null : simInfo.info.flags || null }; }, stateSchema: { @@ -151,6 +156,10 @@ userAgent: NullableIO( StringIO ), window: NullableIO( StringIO ), referrer: NullableIO( StringIO ), + language: NullableIO( StringIO ), + pixelRatio: NullableIO( StringIO ), + isWebGLSupported: NullableIO( BooleanIO ), + checkIE11StencilSupport: NullableIO( BooleanIO ), flags: NullableIO( StringIO ) } } ); Index: phet-io/api/gravity-and-orbits.json IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io/api/gravity-and-orbits.json b/phet-io/api/gravity-and-orbits.json --- a/phet-io/api/gravity-and-orbits.json (revision d79d46bbd619a9ba014b0835aa8b033881ff1a1f) +++ b/phet-io/api/gravity-and-orbits.json (date 1625150422743) @@ -261,9 +261,13 @@ "simInfo": { "_data": { "initialState": { + "checkIE11StencilSupport": null, "dataStreamVersion": "1.0.0", "flags": null, + "isWebGLSupported": null, + "language": null, "phetioCommandProcessorProtocol": "1.0.0", + "pixelRatio": null, "randomSeed": null, "referrer": null, "repoName": "gravity-and-orbits", @@ -26786,9 +26790,13 @@ "methods": {}, "parameterTypes": [], "stateSchema": { + "checkIE11StencilSupport": "NullableIO", "dataStreamVersion": "StringIO", "flags": "NullableIO", + "isWebGLSupported": "NullableIO", + "language": "NullableIO", "phetioCommandProcessorProtocol": "StringIO", + "pixelRatio": "NullableIO", "randomSeed": "NullableIO", "referrer": "NullableIO", "repoName": "StringIO", ```
samreid commented 3 years ago

Looks great, good recommendation. I applied the patch, tested and regenerated API files.