search

phetsims / vegas

Reusable game components for PhET simulations.
MIT License
1 stars 4 forks source link

How to handle gameLevels in the Preferences dialog #113

Closed amanda-phet closed 1 year ago

amanda-phet commented 1 year ago

For the work in https://github.com/phetsims/number-play/issues/189 we are making the option to customize game levels accessible from the preferences dialog. Since this is the first sim to have this in the preferences menu, but gameLevels is in numerous sims, we'd like input from any interested parties as to how this should be controlled.

Currently, they are implemented as a list of checkboxes (@chrisklus can you add a screenshot please?). There might be other considerations that we haven't thought of, so labeling this for design meeting. Assigning all who should participate, at a minimum. Feel free to add thoughts to this issue.

pixelzoom commented 1 year ago

A few thoughts:

amanda-phet commented 1 year ago

On Slack, @kathy-phet said:

There are a few motivations to add these to the preferences menu, and particularly for Number Play.

  1. On any of the apps (iOS, Android, etc), there is no way to change the Game Levels available with the query parameter. So, just as we have with other query parameters, making this available and elevating its possibility with the preferences menu seems like a way to address this issue that has been brought up by Diana and others.
  2. Number Play is for little kids, and their learning helpers - parents, teachers - may desire to constrain the levels for them so they stick to a particular level or game. These two are the top reasons for this, imo. However, this is also consistent with making query parameter functionality available through PhET-iO
pixelzoom commented 1 year ago

Thanks @kathy-phet, that’s helpful.

Regarding:

  1. Number Play is for little kids, and their learning helpers - parents, teachers - may desire to constrain the levels for them so they stick to a particular level or game.

Keep in mind that, unlike apps or desktop applications, sim Preferences are not persistent. So every time the sim is loaded, parent/teacher/student will need to re-set desired Preferences.

pixelzoom commented 1 year ago

Currently, they are implemented as a list of checkboxes (@chrisklus can you add a screenshot please?).

As I noted in https://github.com/phetsims/vegas/issues/113#issuecomment-1282583637, that's a problematic UI. What happens if all checkboxes are unchecked? It's not a good UX. And does the code even handle that correctly?

chrisklus commented 1 year ago

@chrisklus can you add a screenshot please?)

image

@pixelzoom asked:

What happens if all checkboxes are unchecked? It's not a good UX.

Agreed that it's weird.

And does the code even handle that correctly?

When I added an option for gameLevelsProperty in LevelSelectionButtonGroup, removing an assertion was all that was needed for it to be handled correctly.

Here's a patch for these changes:

Note that the layout needs some work and all cases are not centered correctly.

```diff Index: vegas/js/LevelSelectionButtonGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/vegas/js/LevelSelectionButtonGroup.ts b/vegas/js/LevelSelectionButtonGroup.ts --- a/vegas/js/LevelSelectionButtonGroup.ts (revision 3fc13eed3ff0c00fb8b4c5049932fa2860b0479b) +++ b/vegas/js/LevelSelectionButtonGroup.ts (date 1665668850669) @@ -25,6 +25,7 @@ import TProperty from '../../axon/js/TProperty.js'; import Tandem from '../../tandem/js/Tandem.js'; import vegas from './vegas.js'; +import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js'; // Describes one LevelSelectionButton export type LevelSelectionButtonGroupItem = { @@ -60,9 +61,16 @@ // query parameter. Set this to the value of the gameLevels query parameter, if supported by your sim. // See getGameLevelsSchema.ts. gameLevels?: number[]; + + // Same behavior as the gameLevels option except the visibility of the buttons is updated whenever the Property + // changes. + gameLevelsProperty?: TReadOnlyProperty; }; -export type LevelSelectionButtonGroupOptions = SelfOptions & StrictOmit & PickRequired; +export type LevelSelectionButtonGroupOptions = + SelfOptions + & StrictOmit + & PickRequired; export default class LevelSelectionButtonGroup extends Node { @@ -78,7 +86,8 @@ assert && assert( items.length > 0, 'at least one item must be specified' ); const options = optionize, NodeOptions>()( { + StrictOmit, + NodeOptions>()( { // The default layout is a single row of buttons. flowBoxOptions: { @@ -111,11 +120,11 @@ // Hide buttons for levels that are not included in gameLevels. // All buttons must be instantiated so that the PhET-iO API is not changed conditionally. if ( options.gameLevels ) { - assert && assert( options.gameLevels.length > 0, 'at least 1 gameLevel must be visible' ); - assert && assert( _.every( options.gameLevels, gameLevel => ( Number.isInteger( gameLevel ) && gameLevel > 0 ) ), - 'gameLevels must be positive integers' ); - buttons.forEach( ( button, index ) => { - button.visible = options.gameLevels!.includes( index + 1 ); + LevelSelectionButtonGroup.setButtonsVisibility( buttons, options.gameLevels ); + } + if ( options.gameLevelsProperty ) { + options.gameLevelsProperty.link( gameLevels => { + LevelSelectionButtonGroup.setButtonsVisibility( buttons, gameLevels ); } ); } @@ -149,6 +158,18 @@ `invalid level: ${level}` ); this.buttons[ level - 1 ].focus(); } + + /** + * Sets the visibility of each level button based on if the corresponding level number is included in the provided + * gameLevels array. + */ + private static setButtonsVisibility( buttons: Node[], gameLevels: number[] ): void { + assert && assert( _.every( gameLevels, gameLevel => ( Number.isInteger( gameLevel ) && gameLevel > 0 ) ), + 'gameLevels must be positive integers' ); + buttons.forEach( ( button, index ) => { + button.visible = gameLevels.includes( index + 1 ); + } ); + } } vegas.register( 'LevelSelectionButtonGroup', LevelSelectionButtonGroup ); \ No newline at end of file Index: number-play/js/game/view/NumberPlayGameLevelSelectionButtonGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/number-play/js/game/view/NumberPlayGameLevelSelectionButtonGroup.ts b/number-play/js/game/view/NumberPlayGameLevelSelectionButtonGroup.ts --- a/number-play/js/game/view/NumberPlayGameLevelSelectionButtonGroup.ts (revision b2a0d6db924ac9d592af7d1c73b024474e024c0a) +++ b/number-play/js/game/view/NumberPlayGameLevelSelectionButtonGroup.ts (date 1666117870308) @@ -48,7 +48,7 @@ spacing: BUTTON_SPACING } ); }, - gameLevels: numberPlayPreferences.gameLevelsProperty.value, + gameLevelsProperty: numberPlayPreferences.gameLevelsProperty, tandem: Tandem.REQUIRED };
pixelzoom commented 1 year ago

I reviewed and tested the above patch. This is a good start. But it's not ready for production.

In NumberPlay:

In Fourier (a PhET-iO sim):

Other general problems:

So... There are a lot of issues still to be addressed. And I'm still questioning the value, appropriateness, and prioritization of adding gameLevels to Preferences. Is it really do bad if this can only be initialized by a static gameLevels query parameter, and students need to otherwise just ignore the LevelSelectionButtons for levels that they're not interested in? And does this really need to be added now, when PhET has so many other major features that are not completed and blocking publication?

pixelzoom commented 1 year ago
chrisklus commented 1 year ago

In NumberPlay:

  • I'm running the sim from phetmarks with no gameLevels query parameter. The patch is not working for me. When changing Preferences, I don't see any change to the LevelSelectionButtonGroup. All LevelSelectionButtons remain visible regardless of the Preferences settings. All of the descriptions in the GameInfo dialog also remain visible. What am I doing wrong?

Hm, I'm not sure what the problem is. I just tried the patch again and it's working for me. Running without gameLevels, the LevelSelectionButtons load with their default visibility and the preference controls can change the visibility. Running with gameLevels, the LevelSelectionButtons load with the specified configuration and the preference controls reflect the starting state. Maybe double check your transpiler is picking up the changes?

In Fourier (a PhET-iO sim):

  • There is nothing in the patch that addresses PhET-iO design considerations. If gameLevelsProperty is now responsible for the visibility of game levels, then a PhET-iO client should not be able to independently change the visibility of things related to game levels. For example, running Fourier in Studio, I can hide the LevelSelectionButtons via their visibleProperty. This makes it possible for the state of the sim to get out-of-sync with gameLevelsProperty, which will result in serious problems during initialization and record/playback.

Right, makes sense to me. Likely the PhET-iO client should have control of the preferences model instead of the Node visibility of the buttons. I haven't pursued trying this out before discussing this feature any more.

  • If I happen to be on (for example) "Counting: Level 1", and I open the Preferences dialog, then hide "Counting: Level 1", nothing happens. Have you considered this case? If the level that's currently shown is becomes hidden by changing Preferences, then the sim should probably go back to the LevelSelectionButtonGroup. If there's a GameTimer running, it needs to be stopped. Etc. None of that can currently be handled in common code. And it will be a shame if we need to duplicate that in sims, since the whole point of the new UI components that I developer in Q3 2022 was to remove such duplication.

  • Related to the previous bullet... In many sims, there is a Property for the selected game level. For example, in equality-explorer it's SolveItModel levelProperty: Property<SolveItLevel>. What happens when the value of this Property is a level that is made invisible via Preferences?

I did not think about that case. Agreed that it would be a shame to duplicate handling that in all sims.

  • I don't see any generalized UI component for adding gameLevels to the Preferences dialog. I suspect that it's currently specific to Number Play. Or maybe it's just not including in the patch, and lives in a vegas branch that doesn't have a GitHub issue.

It is indeed currently specific to Number Play. The view could certainly be generalized but unfortunately I think the model Properties would make more sense as sim-specific. Unless we created an unnamed list of them in a loop.

So... There are a lot of issues still to be addressed. And I'm still questioning the value, appropriateness, and prioritization of adding gameLevels to Preferences. Is it really do bad if this can only be initialized by a static gameLevels query parameter, and students need to otherwise just ignore the LevelSelectionButtons for levels that they're not interested in? And does this really need to be added now, when PhET has so many other major features that are not completed and blocking publication?

I'm very convinced on this sentiment as well. I think all this work and sim-specific changes are not worth the effort of removing/adding levels during runtime.

If we need the ability to hide/show levels on all platforms, I think time would be better spent (though probably not right now) developing a website and/or app tool that customizes what gameLevels the sim launches with since gameLevels is already supported, is persistent with the URL, and avoids these weird UX cases that happen with runtime changes. Though my opinion may change depending on what comes of the "goals for Preferences dialog" discussion.

amanda-phet commented 1 year ago

We had a general discussion today about the sim preferences dialog (notes here) and indirectly came to the conclusion that gameLevels should not be available in the preferences dialog. We think this customization is better served by using a query parameter, and longer term, a customization user interface on the website (and hopefully the app, too) that would allow someone to customize a sim before starting it up.