Instrument the game screen for PhET-iO

samreid commented 7 years ago

@kathy-phet said:

Perhaps goal of having Build an Atom game instrumented by end of summer -- so we can figure out whether PhET-iO is working for games by then?

The game screen is mostly instrumented using the traditional approach, but part of this process will be checking to make sure that things work as desired and to see if there are new features required for this support. Having @jbphet involved in this step would be great since he has the most knowledge of the game implementation.

Tagging @ariel-phet and @kathy-phet for priority and @jbphet to put it on his radar.

ariel-phet commented 7 years ago

@samreid @jbphet I think priority should wait until expression exchange is published (or at least in the RC process)

zepumph commented 7 years ago

@jbphet I'm happy to be a part of this as well if needed, just let me know. I'm always excited for some uncharted terrain!!!

samreid commented 7 years ago

I added a wrapper that exercises some PhET-iO api features. We could still investigate adding buttons and controls that let you choose how many problems and which level type (level type is currently hard-coded).

samreid commented 7 years ago

I tagged @zepumph because it will probably be good to collaborate on parts of this.

We probably also need to hide the "Start Over" button (if this becomes a real example of how to deal with games).

samreid commented 7 years ago

One thing that may be different for games is that many tandems are random or delayed (not available on startup)--this won't cause problems for the data stream but it could make customization and generating documentation tricky.

zepumph commented 7 years ago

One thing that may be different for games is that many tandems are random or delayed (not available on startup)--this won't cause problems for the data stream but it could make customization and generating documentation tricky.

Agreed. I feel like this has a potential to be a problem for other places in sims as well, like if there is a default array of dynamic particles that aren't created until you click a button or something.

I think the best way to proceed here is to dive in and see the shortcomings first hand. That will give us a better idea on how to fix them. I think that a poorly documented, but fully instrumented BAA Game may be the first goal. I'm free through Thursday and time around meetings.

samreid commented 7 years ago

What we have been doing for PhET-iO is "instrumenting what is easy and seeing what that makes possible." @pixelzoom recommended the opposite approach of "deciding what behavior we would like to enable, then implementing that." (I am paraphrasing). @kathy-phet I am presuming that for games we will want to be able to:

Collect a data stream
Start a single level or single challenge, and see what the student did and what the right answer was.
Allow customization--timer on/off, sound on/off, get rid of the "start over" button

Can we have a design meeting to see what else we want to tackle with a PhET-iO Build an Atom Game? Do we want to make it possible to create game level selection screens with a subset of levels available? Or be able to set a specific challenge (rather than a specific type of challenge)?

samreid commented 7 years ago

At yesterday's status meeting, @kathy-phet said @arouinfar can schedule a design meeting for this on Thursday August 24.

samreid commented 7 years ago

@arouinfar do you think it will be possible to schedule this meeting for 1pm-2pm Thursday August 24? More details in slack.

arouinfar commented 7 years ago

@arouinfar do you think it will be possible to schedule this meeting for 1pm-2pm Thursday August 24? More details in slack.

Sure thing @samreid. I've set a reminder to schedule Build and Atom PhET-iO discussion for 1-2 at the 8/24 design meeting.

samreid commented 7 years ago

I'm a bit confused between setAllowedProblemTypesByLevel, startGameLevel and it seems there is an implicit correspondence between level type and level index which would be good to discuss with @jbphet once we start on this.

samreid commented 7 years ago

Here are the requested features from today's meeting: we should try to order these from easy to difficult and try to quantify (1) = less than an hour and straightforward (10) = I'm not even sure that's possible. We can also quantify the value. So we'll update/edit this issue. Then starting next week (when @zepumph is back), @jbphet will dedicate 1/2 day per week to collaborate on this.

Timer button should be hidable Sound button should be hidable Timer on should be settable Sound on should be settable Make it possible to show a subset of level types on the choose your game screen. Choose the type of challenge with a random challenge instance Choose an exact challenge instance (On the far end of instrumentation) Make it possible to change the number of challenges anywhere between 1 and some max Get rid of the start over button instead of 2 tries, make it possible to have only 1 try or unlimited tries Make it possible to hide the score Make it possible to hide the entire scoreboard Update the number of stars based on the number of challenges Control random seed so all students get same challenges Kathy’s “start with these” list: being able to choose which levels show on the game homescreen, and which are hidden. Timer and sound Then being able to manipulate scoreboard and get rid of score and start over. Changing the number of challenges in a level (And changing the type of challenge within each level) Kathy: It may increase the complexity significantly to be able to specify exact challenges, we should investigate how difficult it would be and go for it if not too complex Make the phet button unclickable Make sure it shows a data stream what the user did even before they pressed submit assessment mode: the student doesn’t know if they got it right or wrong priority: after CCK It’s important in terms of understanding how this impacts the overall approach to phet-io, will we use TTypes, etc. Kathy recommends 1/2 day for JB starting when MK returns

samreid commented 7 years ago

On second thought, let's leave the preceding comment as "raw notes" then use this next comment to organize everything in a table:

samreid commented 7 years ago

Feature	Value	Complexity	Complete
Timer button should be hidable			✓
Sound button should be hidable			✓
Timer on should be settable			✓
Sound on should be settable			✓
Make it possible to show a subset of level types on the choose your game screen.			✓ but layout doesn't reflow
Get rid of the start over button			✓
Make it possible to hide the score			✓
Make it possible to hide the entire scoreboard			✓
being able to choose which levels show on the game homescreen, and which are hidden. Timer and sound			✓
Then being able to manipulate scoreboard and get rid of score and start over.			✓
Make the phet button unclickable			✓
Make it possible to change the number of challenges anywhere between 1 and some max (or unlimited)			✓via query string
Choose the type of challenges with a level			✓ via api call
Choose an exact challenge instance @jbphet			✓
Make sure it shows a data stream what the user did even before they pressed submit			✓
assessment mode: the student doesn’t know if they got it right or wrong @jbphet			✓ (see questions)
Update the number of stars based on the number of challenges OR allow to be separately configured via query parameter			✓
Control random seed so all students get same challenges			✓ via query parameter or API call, and also possible to get seed when it was random
Better tandem names for periodic table buttons? (see PeriodicTableNode.js)			✓
Dealing with random/dynamic tandems in the API (for customization,data stream, API, API diff) see https://github.com/phetsims/phet-io/issues/1201
Add full support for saving/restoring state

samreid commented 7 years ago

I made this much progress, getting to a good point where it would be good to collaborate with @jbphet:

@jbphet can we set up a time for this week? My schedule is pretty open 9:30am-2:30pm M,Tu,W,F

samreid commented 7 years ago

Questions:

In assessment mode, what should show when all questions completed?
Should data stream show details when new challenge was created?

samreid commented 6 years ago

@jbphet and I have a schedule to work on this, not planning to work on it in the meantime, so unassigning myself.

samreid commented 6 years ago

[x] Do we need save/load state for this screen?

Yes, we decided in the 9/14/2017 design meeting that we should support save/load.

samreid commented 6 years ago

[ ] The data stream is showing a user event causing another user event:

build-an-atom-game.html?sim=build-an-atom:78 {
  "messageIndex": 76,
  "eventType": "user",
  "phetioID": "buildAnAtom.gameScreen.view.challengeView_0.schematicToElementChallengeView.periodicTable.row_1.column_7.buttonListener",
  "componentType": "TButtonListener",
  "event": "fire",
  "time": 1505319366789,
  "children": [
    {
      "messageIndex": 77,
      "eventType": "user",
      "phetioID": "buildAnAtom.gameScreen.view.challengeView_0.schematicToElementChallengeView.periodicTable.row_1.column_7",
      "componentType": "TPeriodicTableCell",
      "event": "fired",
      "time": 1505319366789,
      "children": [

We saw this when clicking on a periodic table button.

samreid commented 6 years ago

[ ] To @kathy-phet and @ariel-phet: What to do for "impossible" atoms that the user tries to create? Like trying to have more than 10 electrons in an atom schematic?
[ ] Do we need a better API for starting specific challenges? Right now it is possible but takes 2 calls.

samreid commented 6 years ago

[ ] For @kathy-phet and @ariel-phet: is it OK to have things done in query parameters? Or should it be unified under PhET-iO API calls?

kathy-phet commented 6 years ago

Better to have everything under API calls -- although having some available also as query is fine. I assume Query parameter calls are things that can be leveraged in regular PhET sims?

samreid commented 6 years ago

Yes, query parameters could be leveraged in both PhET and PhET-iO sims. So during our design process, we should decide what features would be needed in the PhET sims, if any.

zepumph commented 6 years ago

It looks like phet-io fuzzTesting is failing from our work today, I'm not sure what's going on there.

samreid commented 6 years ago

It looks like we neglected to commit the predeterminedChallenges[], I did so in the preceding commit. Local ?fuzzMouse seems OK.

zepumph commented 6 years ago

Notes about game state saving from the session today:

The state of the BAA game is potentially uncharted territory. Basically we want to ability to set the array of challenges, but then those are static for the lifetime of that level (I think). So they are pretty much static, but with a caveat that we want to be able to set them dynamically on the downstream sim.
@jbphet, @samreid and I looked at instrumenting the challengeSetProperty. This Property.<Array.<BAAGameChallenge>> proved challenging with the normal instrumentation strategy. We probably want to look at the BAAGameModel creating its own challenges so that we have more control about how the dynamic challenges are created and then updated.
References. It seems like PhET-iO should find a more general pattern for saving references in the upstream sim to set on the downstream sim. Rather than having a type for each type of reference (like TTrackReference. This will potentially help us with creating BAAGameChallenges because you have to pass in a reference to the model.

We will work again on this tomorrow.

zepumph commented 6 years ago

Notes from meeting today about TTypes in general:

Possibility of putting the TType (and potentially the tandem) onto the instance. This seemed to be an improvement for TArray if we want to support heterogeneous arrays, but we couldn't think of many of uses yet.
If we do that, and deparameterize all of the TTypes, it may make it impossible to deserialize data from the wrapper. Especially TFunctionWrapper seemed very problematic without parameters like returnValue and args.
Putting toStateObject on the instance instead of the TType, but it's pretty flexible to have it on the TType. May be nice on the instance in examples like supporting heterogeneous array.

kathy-phet commented 6 years ago

What does this mean? Thx for explaining.

deserialize data from the wrapper. Especially TFunctionWrapper seemed very problematic without parameters like returnValue and args.

samreid commented 6 years ago

For example, TNode has this PhET-iO method:

    addVisibleListener: {
      returnType: TVoid,
      parameterTypes: [ TFunctionWrapper( TVoid, [ TBoolean ] ) ],
      implementation: function( callback ) {
        var inst = this.instance;
        this.instance.on( 'visibility', function() {
          callback( inst.isVisible() );
        } );
      },
      documentation: 'Adds a listener for when visibility of the node changes'
    },

When the wrapper calls this method, the TFunctionWrapper arguments of TVoid and [TBoolean] tell phet-io how to interpret data from the wrapper (e.g. that there will be one boolean value in this case).

zepumph commented 6 years ago

This week we added fromStateObject onto TBAAGameChallenge and tried to get it to create itself in the downstream sim. We were pretty far along that path and then ran into a problem where some of BAAGameChallenge's Properties weren't being disposed, so we need to make sure that the downstream sim is being cleared completely before creating new challenges there.

Possible the above will work, but @samreid had a suggestion to make each screen have its own master state setting/getting methods, because then the BAAGameModel could handle most/everything that it needed to for the whole game model. We may at somepoint turn towards making that happen.

We did not set another time to meet, but I would assume it won't be until next week since @jbphet was on vacation.

commits are below

p.s. (I didn't commit the challengeType property assignment hack)

zepumph commented 6 years ago

Currently BAA fuzz fails for phet brand but not phet-io because of the above commits:



assert.js:31 enabling assert
Emitter.js?bust=1508194895635:103 Assertion failed: tried to removeListener on something that wasn't a listener
Emitter.js?bust=1508194895635:103 Uncaught Error: Assertion failed: tried to removeListener on something that wasn't a listener
    at window.assertions.assertFunction (assert.js:22)
    at Emitter.removeListener (Emitter.js?bust=1508194895635:103)
    at DerivedProperty.unlink (Property.js?bust=1508194895635:251)
    at DerivedProperty.dispose (DerivedProperty.js?bust=1508194895635:96)
    at ParticleAtom.dispose (ParticleAtom.js?bust=1508194895635:265)
    at NonInteractiveSchematicAtomNode.disposeNonInteractiveSchematicAtomNode (NonInteractiveSchematicAtomNode.js?bust=1508194895635:92)
    at NonInteractiveSchematicAtomNode.dispose (NonInteractiveSchematicAtomNode.js?bust=1508194895635:103)
    at NonInteractiveSchematicAtomNode.Node.dispose (Node.js?bust=1508194895635:518)
    at SchematicToSymbolChallengeView.disposeSchematicToSymbolChallengeView (SchematicToSymbolChallengeView.js?bust=1508194895635:64)
    at SchematicToSymbolChallengeView.dispose (SchematicToSymbolChallengeView.js?bust=1508194895635:92)

zepumph commented 6 years ago

That error will be addressed in https://github.com/phetsims/build-an-atom/issues/176

zepumph commented 6 years ago

Today we worked on the disposal issues of #176.

We then talked about how our current implementation pattern requires a lot of overhead to implement dispose for all instrumented items. This involved beginning with just one single challenge, and then I went in and implemented dispose for everything that was causing a problem when addInstance was called on a phetioID that was already in the instanceMap. I may have gotten a bit carried away after the meeting was over, but if someone has to do it (implement all of the dispose methods). It may as well be 1 of us, rather than putting it off for the next 2 hour meeting.

zepumph commented 6 years ago

Today in BAA meeting with @jbphet and @samreid, we made a good fix to NumberProperty, adding a dispose method so that BAA phet-io fuzz won't fail.

Then we continued to spend a very long time being tired of how many dispose methods we might have to implement. During today's session we found that SymbolToSchematicChallenge has its own BuildAnAtomModel (should be named BAAModel.js for convention) that was not being disposed correctly. So we added that dispose method that looks like:

    dispose: function() {
      // derivedProperty first
      this.nucleusStableProperty.dispose();

      this.showElementNameProperty.dispose();
      this.showNeutralOrIonProperty.dispose();
      this.showStableOrUnstableProperty.dispose();
      this.electronShellDepictionProperty.dispose();
      this.particleAtom.dispose();
      this.buckets.protonBucket.dispose();
      this.buckets.electronBucket.dispose();
      this.buckets.neutronBucket.dispose();
      this.electrons.forEach( function( electron ) { electron.dispose();} );
      this.nucleons.forEach( function( nucleon ) { nucleon.dispose();} );
    },

Then we found this problematic, because BAAGameModel.clearChildInstances() is calling this method before it's view has been disposed, so we are running into an error with previousView.dispose() in BAAGameScreenView because the underlying model is already disposed. An ugly problem since phet-io setState usually doesn't care about order at all. That was where we ended.

We also got talking about implementing some sort of "tree removal" to be used when you have an object with a ton of tandem instances as "children" (quotes since tandems ids aren't really nested) to all have removeInstance() called on them at once, basically a recursive way to dispose an object for phet-io state. Here is our first pass implementation:


    treeRemoveInstance: function( tandem ) {
      var self = this;
      var toBeRemoved = [];
      var registeredTandemIds = Object.keys( instanceMap );
      registeredTandemIds.forEach( function( tandemKey ) {
        if ( tandemKey.indexOf( tandem.id ) === 0 ) {
          toBeRemoved.push( tandemKey );
        }
      } );

      toBeRemoved.forEach( function( phetioID ) {
        var instance = instanceMap[ phetioID ].instance;
        self.removeInstance( phetioID, instance );
      } );
    },

A problem is that this will cause memory leaks if nested components need to un-register anything more than tandems. Perhaps not a big deal if this is only used where there shouldn't be any other disposal needs.

samreid commented 6 years ago

I wonder if we added an aboutToBeDisposedEmitter, the model could use it to signify to the view to dispose itself. Would that sort out the ordering issue?

zepumph commented 6 years ago

@jbphet @samreid and I are feeling like this job is very complex because of disposal order in terms of view/model. One thought would be to try to instrument the next game under development, as it was being created, to help see if this was a unique challenge because of how BAA was implemented, or if this has to do with shortcomings of phet-io.

zepumph commented 6 years ago

I just found a bug where you do a whole level, and then I tried to start that level again: Assertion failed: challenges should be cleared before starting a new game

jbphet commented 6 years ago

Today we resolved some issues related to the reward node that is shown when a level is completed. Next time we should work on making the stars update correctly in the level selection button (see #179) and then work on the problem where a new level can't be started once a level is completed.

samreid commented 6 years ago

It's been great to have @jbphet involved in this process, I'll unassign until we are collaborating on this again.

jbphet commented 6 years ago

Work seems to have ground to a halt on this effort, and there are some problems occurring due to the partially implemented code (see #185). I'm going to reassign this to @samreid since he is the PhET-iO tech lead, and he can work with the powers that be to determine if and when work on this should resume.

samreid commented 6 years ago

@ariel-phet can you comment on proposed timeline for PhET-iO API design and implementation for this sim?

ariel-phet commented 6 years ago

@samreid I honestly cannot comment, and I do not know timeline. marking the issue as deferred for the moment until @kathy-phet decides it is a priority. My suggestion would be to revisit once @jbphet has published EFAC. Assigning you to be aware of that suggestion, feel free to clear assignees or get @jbphet to make a calendar reminder or such.

samreid commented 6 years ago

@jbphet how do you feel about a calendar reminder for checking on this after EFAC? We would start with a PhET-iO design process, deciding on the PhET-iO specific features as a new first step.

jbphet commented 6 years ago

I've added a calendar reminder for next Jan, since that's when EFAC is supposed to be out. Unassigning for now.

zepumph commented 4 years ago

The game screen does not (still) support state, and is not set up to be maintained for phet-io yet. As @samreid and I work on updating phet-io tests, we decided to remove that screen from the PhET-iO sim above.

zepumph commented 3 years ago

From https://github.com/phetsims/build-an-atom/issues/156#issuecomment-324743529

being able to choose which levels show on the game homescreen, and which are hidden. Timer and sound

This was the request from a user over in https://github.com/phetsims/build-an-atom/issues/222

zepumph commented 3 years ago

Presumably we decided that this was covered by PhET-iO because you can hide each button that was desired (with the visibleProperty). But this isn't supported via QP right now, all 4 are always created:

https://github.com/phetsims/build-an-atom/blob/fd8338e73c7a4df7c39e536ecfd870fa205913c2/js/game/view/StartGameLevelNode.js#L51-L80

phetsims / build-an-atom

Instrument the game screen for PhET-iO #156