search

phetsims / vegas

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

Create a reusable Level-Selection UI. #108

Closed pixelzoom closed 1 year ago

pixelzoom commented 2 years ago

From https://github.com/phetsims/vegas/issues/104#issuecomment-1125393164.

The UI might include only the level-selection buttons, or it might include other UI optional components (title, info button, sound button, timer button,...)

It would be nice to have this done by mid June, for use in Number Play.

pixelzoom commented 2 years ago

This seems like an excellent place to try scenery's new GridBox. Here's an untested sketch.

type SelfOptions = {};
type LevelSelectionBoxOptions = SelfOptions & Omit<GridBoxOptions, 'children'>;

class LevelSelectionBox extends GridBox {

  constructor( buttons: LevelSelectionButton[], providedOptions?: LevelSelectionBoxOptions ) {

     const options = optionize< LevelSelectionBoxOptions, SelfOptions, GridBoxOptions>()( {
       // ...
     }, providedOptions );

     options.children = buttons;

     super( options );
  }
}

Althought this isn't really doing much .... Maybe wrap this in another class that adds title and optional Info button?

pixelzoom commented 2 years ago

As I think about this more, I'm moving closer to the conclusion that this UI component may not by desirable. There are too many things about it that can differ between sims, and (likely) little standardization in how things have been designed.

Parameters:

pixelzoom commented 2 years ago

I thought I'd start by creating a subclass of GridBox to handle layout of LevelSelectionButtons, and be responsible for setting visibility of those buttons based on the gameLevels query parameter.

I ran into all kinds of problems with GridBox, which I reported to @jonathanolson in Slack, and in https://github.com/phetsims/scenery/issues/1430. So this issue is on-hold until those problems are resolved.

Here's where I was with LevelSelectionBox.ts when I stopped:

LevelSelectionBox.ts // Copyright 2018-2022, University of Colorado Boulder /** * LevelSelectionBox provides the layout a set of LevelSectionButtons. * If the simulation supports the gameLevels query parameter (see getGameLevelsSchema.ts) the caller * can optionally provide options.gameLevels to control which buttons are visible. * * @author Chris Malley (PixelZoom, Inc.) */ import { GridBox, GridBoxOptions } from '../../scenery/js/imports.js'; import vegas from './vegas.js'; import optionize from '../../phet-core/js/optionize.js'; import StrictOmit from '../../phet-core/js/types/StrictOmit.js'; import LevelSelectionButton from './LevelSelectionButton.js'; type SelfOptions = { // Game levels whose buttons should be visible. Levels are numbered starting from 1. // This is typically set to the value of the gameLevels query parameter. See getGameLevelsSchema.ts. gameLevels?: number[]; }; export type LevelSelectionBoxOptions = SelfOptions & StrictOmit; export default class LevelSelectionBox extends GridBox { public constructor( buttons: LevelSelectionButton[], providedOptions?: LevelSelectionBoxOptions ) { const options = optionize, GridBoxOptions>()( { // GridBoxOptions autoColumns: 2, // the default is to put all buttons in one row xSpacing: 20, ySpacing: 20, children: buttons }, providedOptions ); // Hide buttons for levels that are not included in gameLevels. // We must still create these Nodes so that the PhET-iO API is not changed. if ( options.gameLevels ) { 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 ); } ); } super( options ); } } vegas.register( 'LevelSelectionBox', LevelSelectionBox );
pixelzoom commented 2 years ago

@jonathanolson and I discussed a plan for addressing the GridBox issue in https://github.com/phetsims/scenery/issues/1430.

@jonathanolson also suggested that I might consider a FlowBox with a maximum width, that wraps to the next line. That might be a better bit for the layout of LevelSelectionButtons, since we're typically want use multiple rows only if the layout gets too wide. But when hiding some buttons (either via ?gameLevels or via PhET-iO) we could end up with layout that are not aesthetically pleasing, like 4 button on the first row, and 1 button on the second row.

pixelzoom commented 2 years ago

On hold until https://github.com/phetsims/scenery/issues/1429 is addressed.

pixelzoom commented 2 years ago

https://github.com/phetsims/scenery/issues/1429 and https://github.com/phetsims/scenery/issues/1430 have been addressed, so removing on-hold label.

pixelzoom commented 2 years ago

Above I said:

I thought I'd start by creating a subclass of GridBox to handle layout of LevelSelectionButtons, and be responsible for setting visibility of those buttons based on the gameLevels query parameter.

After running into problems with scenery layout Nodes, and rolling this around in my brain for a couple of weeks... I think that's the wrong approach, Coming up with a layout that makes everyone happy is unlikely, time consuming, etc.

So I'm thinking that a better approach would be to create LayoutSelectionButtonGroup, similar to other PhET "groups" (RectangularRadioButtonGroup, etc.), with a customizable layout.

LayoutSelectionButtonGroup would have:

pixelzoom commented 2 years ago

I've started integrating LevelSelectionButtonGroup into sims, but there are blocking problems with scenery layout. I'm discussing these problems with @jonathanolson via Slack.

In the meatime, any sim that uses LevelSelectionButtonGroup should be considered blocked from publication.

pixelzoom commented 2 years ago

I've started integrating LevelSelectionButtonGroup into sims, but there are blocking problems with scenery layout. I'm discussing these problems with @jonathanolson via Slack.

@jonathanolson and I discussed on Slack. I was getting confused about "primary direction" and "secondary direction", and semantics of the term "line" in a layout context. @jonathanolson clarified, and explained the benefits. There are no changes to scenery layout required.

pixelzoom commented 2 years ago

This work is done and ready for review.

Summary:

screenshot_1769
pixelzoom commented 2 years ago

One aspect of this group that I did not address is voicing/description. Support for these features is currently inconsistent (or non-existent) across PhET groups. Here's the current status:

Since there's no pattern here, I've chosen to do nothing for LevelSelectionButtonGroup. When voicing/description features are needed in a sim that has a game, this will need to be addressed. More in https://github.com/phetsims/sun/issues/775 ...

pixelzoom commented 2 years ago

@kathy-phet please assign a reviewer for this work. Note that until this is reviewed, publication of sims that use LevelSelectionButtonGroup is blocked.

pixelzoom commented 2 years ago

In the above commits, I improved the demo of LevelSelectionButtonGroup. See screenshot below. It now includes:

screenshot_1776
pixelzoom commented 2 years ago
  • I converted all of my sims to use LevelSelectionButtonGroup: fourier-making-waves, balancing-chemical-equations, reactants-products-and-leftovers, graphing-lines, graphing-slope-intercept. Other sims will be converted by their responsible developers.

The other sims are:

@kathy-phet consider choosing one of the above developers to review LevelSelectionButtonGroup. And as part of that review, ask them to convert one or more of their sims.

chrisklus commented 2 years ago

@kathy-phet asked me to review this earlier today, self assigning.

pixelzoom commented 2 years ago

Thanks @chrisklus. I realize that this is a long issue (lots of comments). You should be able to proceed by reading only these 4 comments, the first of which is a summary:

chrisklus commented 2 years ago

LevelSelectionButtonGroup.ts is looking great and the gameLevels query parameter is working nicely. I had a good experience using this in Number Play. A couple notes:

  1. Can levelSelectionButtonOptions be optional? I ended up putting most options in item.options and could see how levelSelectionButtonOptions may not be used in some cases.

  2. Default layout got me pretty far but not quite what was in place in Number Play before, see https://github.com/phetsims/number-play/issues/184, and I did not see an easy way to fix this with a custom layout. I tried creating a type that tacks on info about the level (like game type) for the button so that I could split the buttons into two types for the layout, but naturally createLayoutNode is strict about its param type LevelSelectionButton[]. Any ideas on how to achieve this behavior? Or maybe it should not be a feature of LevelSelectionButtonGroup to know things about the levels the buttons are for? Worst case I could distinguish with getBaseColor from ButtonNode, but that seems not ideal. I'll add this as a comment in the issue so we can continue discussion over there.

Back to @pixelzoom for any changes/thoughts from the items above and feel free to close if all set.

pixelzoom commented 2 years ago
  1. Can levelSelectionButtonOptions be optional?

Yes, good catch. Done in the above commit.

  1. Default layout got me pretty far but not quite what was in place in Number Play before ...

@chrisklus and I discussed this via Zoom, notes in https://github.com/phetsims/number-play/issues/184#issuecomment-1226470463.

So I think this is ready to close.

pixelzoom commented 1 year ago

For the sims identified in https://github.com/phetsims/vegas/issues/108#issuecomment-1183478528, I've created sim-specific issues to convert to LevelSelectionButtonGroup. See issues linked above this comment.

zepumph commented 1 year ago

Looks like one more issue tied to this issue, https://github.com/phetsims/balancing-chemical-equations/blob/b099c6f828c8de1f7a8a36c69f48677ec149fb03/js/game/view/LevelSelectionNode.js#L74

From https://github.com/phetsims/chipper/issues/1372

pixelzoom commented 1 year ago

The TODO in https://github.com/phetsims/balancing-chemical-equations/blob/b099c6f828c8de1f7a8a36c69f48677ec149fb03/js/game/view/LevelSelectionNode.js#L74 was specific to BCE, not this issue. Re-closing.