partial stars can be hard to see

[jbphet]

On Arithmetic, if the user doesn't get all the answers correct, the number of stars shown as fully filled in is scaled based on the number of correct answers vs the number divided by the total number of questions. In the worst case, this can be 143/144, and this is very hard to see in the stars. Screenshot:

This should probably be changed to be more "chunky" so that there is never less than a half start that is not filled in.

[jbphet]

This should be addressed the next time Arithmetic is built and released off of master.

[marlitas]

This seems like it might be a relatively straightforward fix and can be part of the upcoming Arithmetic character sets publication.

[amanda-phet]

@marlitas and I discussed this. I think it is fine to have partial stars up to 4.5 stars (130/144), and then it should remain filled in at 4.5 until they get 144/144 and then it's 5 full stars.

[Luisav1]

I added a new option halfStarScoreRange in ScoreDisplayStars to only half-fill a star when the score is in a certain range. Since this involved common-code changes I'd like @marlitas or @AgustinVallejo to review this before closing, thanks.

[amanda-phet]

Discussed 12/7/23. Don't feel strongly about changing the logic for these stars, but since @Luisav1 created some good logic then I will define ranges for 0, .5, 1, etc. stars.

[marlitas]

@Luisav1 the commits look really good. We might need to make some additional changes based on a design meeting yesterday, but I think you put in a great structure for that.

Assigning to @amanda-phet to answer some questions:

• right now the logic is only shifting to a half star for levels where the table size is 12. Do you want to apply a half star range to all of the tables?
• From the meeting yesterday it sounded like we are moving forward with filling in half increments across all of the stars and not just the last star, is that correct?

[amanda-phet]

right now the logic is only shifting to a half star for levels where the table size is 12. Do you want to apply a half star range to all of the tables?

I noticed that, and it should be for all levels not just the 12x12 levels.

From the meeting yesterday it sounded like we are moving forward with filling in half increments across all of the stars and not just the last star, is that correct?

Correct. So, for level 1 (36 points), I would expect the stars to fill like this:

Stars	Points
.5	1-4
1	5-8
1.5	9-12
2	13-15
2.5	16-19
3	20-23
3.5	24-27
4	28-31
4.5	32-35
5	36

[marlitas]

This patch gets pretty close to the above without hardcoding an array of values. I'm curious if this would be acceptable from a design perspective. It's much more robust in the code this way so I believe this would scale to other sims and scenarios a bit better.

Subject: [PATCH] partial stars
---
Index: arithmetic/js/common/model/LevelModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/arithmetic/js/common/model/LevelModel.js b/arithmetic/js/common/model/LevelModel.js
--- a/arithmetic/js/common/model/LevelModel.js	(revision 4687da27276e8ac94dea162e3b354692b07111f2)
+++ b/arithmetic/js/common/model/LevelModel.js	(date 1707330096766)
@@ -17,19 +17,12 @@
 
   /**
    * @param {number} tableSize - width and height of the multiplication table, which is assumed to be square
-   * @param {Array<number>} halfStarScores - array of scores required when filling in the ScoreDisplayStars in half-star increments.
    */
-  constructor( tableSize, halfStarScores ) {
+  constructor( tableSize ) {
 
     // Only fill in all stars completely when the perfect score is reached, so the last score must be the perfect score.
     // See https://github.com/phetsims/arithmetic/issues/174 for more details.
     const perfectScore = tableSize * tableSize;
-    halfStarScores.push( perfectScore );
-
-    // Assert to ensure stars are filled in half-star increments. See https://github.com/phetsims/arithmetic/issues/174.
-    assert && assert( halfStarScores.length === ArithmeticConstants.NUM_STARS * 2,
-      'There must twice as many scores as the number of stars. Number of scores: ', halfStarScores.length,
-      ' Number of stars: ', ArithmeticConstants.NUM_STARS );
 
     // observable model properties
     this.bestTimeProperty = new Property( null );  // @public - best time for level
@@ -40,7 +33,6 @@
     this.tableSize = tableSize; // @public, read only
     this.perfectScore = perfectScore; // @public, read only
     this.gameTimer = new GameTimer(); // @public - timer for this level
-    this.halfStarScores = halfStarScores; // @public, read only
 
     // @private - 2d array that tracks the 'used' state of each of the cells in the multiplication table for this level,
     // accessed through methods
Index: arithmetic/js/common/view/LevelSelectionNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/arithmetic/js/common/view/LevelSelectionNode.js b/arithmetic/js/common/view/LevelSelectionNode.js
--- a/arithmetic/js/common/view/LevelSelectionNode.js	(revision 4687da27276e8ac94dea162e3b354692b07111f2)
+++ b/arithmetic/js/common/view/LevelSelectionNode.js	(date 1707330096774)
@@ -77,7 +77,7 @@
           createScoreDisplay: scoreProperty => new ScoreDisplayStars( scoreProperty, {
             numberOfStars: ArithmeticConstants.NUM_STARS,
             perfectScore: level.perfectScore,
-            halfStarScores: level.halfStarScores
+            useHalfStarScores: true
           } ),
           soundPlayerIndex: levelIndex
         }
Index: vegas/js/ScoreDisplayStars.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vegas/js/ScoreDisplayStars.ts b/vegas/js/ScoreDisplayStars.ts
--- a/vegas/js/ScoreDisplayStars.ts	(revision a05a7dc58bdc15e86214a3f2da9a16dd16a5e1ce)
+++ b/vegas/js/ScoreDisplayStars.ts	(date 1707330497291)
@@ -15,13 +15,14 @@
 import StarNode, { StarNodeOptions } from '../../scenery-phet/js/StarNode.js';
 import { HBox, HBoxOptions } from '../../scenery/js/imports.js';
 import vegas from './vegas.js';
+import Utils from '../../dot/js/Utils.js';
 
 type SelfOptions = {
   numberOfStars?: number;
   perfectScore?: number;
 
-  // Scores required to fill in the stars in half-star increments.
-  halfStarScores?: Array<number>;
+  // Set this option to true when we want to show score increments through half stars.
+  useHalfStarScores?: boolean;
 
   starNodeOptions?: StarNodeOptions;
 };
@@ -39,7 +40,7 @@
       // SelfOptions
       numberOfStars: 1,
       perfectScore: 1,
-      halfStarScores: [],
+      useHalfStarScores: false,
       starNodeOptions: {
         starShapeOptions: {
           outerRadius: 10,
@@ -75,35 +76,13 @@
       let remainder = proportion * numberOfStars - numFilledStars;
 
       // Fill stars in using half-star increments.
-      if ( options.halfStarScores.length !== 0 && numberOfStars === 5 ) {
-
-        // Set the remainder for scores that would have a half-star fill, otherwise, no half-stars (remainder = 0).
-        remainder = 0;
-        const scoreArray = options.halfStarScores;
-        if ( score >= scoreArray[ 0 ] && score < scoreArray[ 1 ] ||
-             score >= scoreArray[ 2 ] && score < scoreArray[ 3 ] ||
-             score >= scoreArray[ 4 ] && score < scoreArray[ 5 ] ||
-             score >= scoreArray[ 6 ] && score < scoreArray[ 7 ] ||
-             score >= scoreArray[ 8 ] && score < scoreArray[ 9 ] ) {
-          remainder = 0.5;
-        }
-
-        // Set the number of filled stars.
-        if ( score >= scoreArray[ 1 ] && score < scoreArray[ 3 ] ) {
-          numFilledStars = 1;
-        }
-        else if ( score >= scoreArray[ 3 ] && score < scoreArray[ 5 ] ) {
-          numFilledStars = 2;
-        }
-        else if ( score >= scoreArray[ 5 ] && score < scoreArray[ 7 ] ) {
-          numFilledStars = 3;
-        }
-        else if ( score >= scoreArray[ 7 ] && score < scoreArray[ 9 ] ) {
-          numFilledStars = 4;
-        }
-        else if ( score === scoreArray[ 9 ] ) {
-          numFilledStars = numberOfStars;
-        }
+      if ( options.useHalfStarScores && remainder !== 0 ) {
+        const numberOfHalves = numberOfStars * 2;
+        const halfStarValue = perfectScore / numberOfHalves;
+        numFilledStars = Utils.roundToInterval( score / ( halfStarValue * 2 ), 1 );
+        console.log( numFilledStars );
+        remainder = score / ( halfStarValue * 2 ) - numFilledStars > 0 ? 0.5 : 0;
+        console.log( remainder );
       }
 
       // Fill stars in.
Index: vegas/js/LevelCompletedNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vegas/js/LevelCompletedNode.ts b/vegas/js/LevelCompletedNode.ts
--- a/vegas/js/LevelCompletedNode.ts	(revision a05a7dc58bdc15e86214a3f2da9a16dd16a5e1ce)
+++ b/vegas/js/LevelCompletedNode.ts	(date 1707330096782)
@@ -35,7 +35,7 @@
   buttonFont?: Font;
   buttonFill?: TColor;
   starDiameter?: number;
-  halfStarScores?: Array<number>;
+  useHalfStarScores?: boolean;
   contentMaxWidth?: number | null; // applied as maxWidth to every subcomponent individually, not Panel's content
 };
 
@@ -71,7 +71,7 @@
       buttonFont: DEFAULT_BUTTON_FONT,
       buttonFill: PhetColorScheme.BUTTON_YELLOW,
       starDiameter: 62,
-      halfStarScores: [],
+      useHalfStarScores: false,
       contentMaxWidth: null,
 
       // PanelOptions
@@ -108,7 +108,7 @@
     const scoreDisplayStars = new ScoreDisplayStars( new Property( score ), {
       numberOfStars: numberOfStars,
       perfectScore: perfectScore,
-      halfStarScores: options.halfStarScores,
+      useHalfStarScores: options.useHalfStarScores,
       starNodeOptions: {
         starShapeOptions: {
           innerRadius: options.starDiameter / 4,
Index: arithmetic/js/common/view/LevelCompletedNodeWrapper.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/arithmetic/js/common/view/LevelCompletedNodeWrapper.js b/arithmetic/js/common/view/LevelCompletedNodeWrapper.js
--- a/arithmetic/js/common/view/LevelCompletedNodeWrapper.js	(revision 4687da27276e8ac94dea162e3b354692b07111f2)
+++ b/arithmetic/js/common/view/LevelCompletedNodeWrapper.js	(date 1707330096784)
@@ -43,7 +43,7 @@
           continueCallback,
           {
             fill: new Color( 255, 235, 205 ),
-            halfStarScores: levelModel.halfStarScores,
+            useHalfStarScores: true,
 
             // Empirically determined values such that LevelCompletedNodeWrapper has same width and proportions as controlPanelVBox.
             starDiameter: 45,

[marlitas]

@Luisav1 and I paired making the above patch even more accurate. It seems to be evenly distributing half stars on it's own calculation which I think will make it more robust and scalable down the line. We had to use some unique rounding to calculate the number of full stars since normally 0.5 would round up to 1, but in this case we only want to render 0.5 still. Because of this a review from @pixelzoom would be greatly appreciated.

@amanda-phet let's find a time to chat through it synchronously to make sure this feels good from a design perspective as well.

Thanks!

[amanda-phet]

This seems to be working as intended. When I get 1 correct, I see 1/2 star. When I get 1-4 correct I still see 1/2 a star. When I get 5 correct I get a full star.

I think it's very possible someone paying close attention to the star calculations could find this behavior strange or buggy. I get 1 star with 5 points. I get 2 stars with 13 points. It seems like 1 star should be closer to 7 points, since 36/5 = 7.2. So, this is my fault for not realizing this when I calculated the score ranges. I think they should actually look more like this:

Stars	Points
.5	1-6
1	7-10
1.5	11-13
2	14-17
2.5	18-20
3	21-24
3.5	25-28
4	29-32
4.5	33-5
5	36

I took the total points (36) divided by number of stars (5) to get the approximate number of points to get 1 full star, so it is a logical jump from 1 to 2 to 3 and so on.

Can this be accomplished instead?

[marlitas]

@amanda-phet let's chat about this, because the code is no longer hard-coding the values that render stars, and I would like to stay away from hard-coding those values. Right now it's calculating the fill based on a proportion that rounds up or down to halves.

[pixelzoom]

This issue seems to be moving forward based on @jbphet's comment in #174 (comment):

This should probably be changed to be more "chunky" so that there is never less than a half start that is not filled in.

I can't say that I agree with that as a general approach. And I don't think handling it in ScoreDisplayStars is the right direction.

I think that showing only half stars is confusing for the same reasons that @amanda-phet mentioned above. And that behavior should not be built into the stars. You accomplish the same thing by making no changes to the stars implementation, and passing the stars a DerivedProperty that maps from score/points to how many fractions of a star to display. If you wanted a mapping that's only half stars, then that would be handled by the DerivedProperty. If sim instead needed a mapping more like what @amanda-phet described in #174 (comment), then the DerivedProperty would handle that mapping. Etc.

So I recommend reverting the changes made to the ScoreDisplayStars, and instead pass a DerivedProperty as the first argument to ScoreDisplayStars constructor -- where the DerivedProperty handles the mapping to display half stars for this sim.

[amanda-phet]

I agree with staying away from hard-coded values. Total points ÷ number of stars is where I got the value and showed the resulting ranges, so it can be a derived value based on those two variables.

I think the underlying issue here is only present when the fill is close to 0 or close to 100%, and instead we should probably solve the underlying issue another way rather than create these odd ranges for half stars.

I'd like to discuss @pixelzoom 's suggestion, or even consider abandoning this issue since I'm not a fan of how it's behaving now.

[pixelzoom]

Happy to discuss further. Perhaps a Zoom call would be more efficient?

[marlitas]

Okay. The thought in putting this in ScoreDisplayStars was that this may want to be a general approach for other sims on top of Arithmetic. I see there's a zoom meeting scheduled. I would also say this might be tied to phetsims/vegas#123 as well since it's starting to delve into the general design of stars.

I will say that I'm not the biggest fan of the current design of the scoreDisplayStars for reasons mentioned in this issue that also combined with the phetsims/vegas#123 issue, however I do recognize that there's a lot of history here I'm unaware of so I think my opinion can be taken as a grain of salt. In general, I feel like the half stars approach is an improvement on the current design, but I don't feel too strongly either way. Especially with the knowledge that game screens re-design is greatly needed in general and our time maybe isn't best spent here right now.

[marlitas]

Meeting 2/12:

• We don't think this is worth the effort. This problem is very unique to Arithmetic since star values are generally not this high unless we are accumulating stars.
• The way half stars fill in feels very confusing and doesn't provide the feedback we'd expect. We much prefer the proportional fill in.
• For this age group having 143/144 as the score display doesn't make sense (or a percentage)
• We would like to revert the above commits, close this issue, and leave arithmetic stars as is.

[marlitas]

All the relevant commits have been reverted, and things are now working as would be expected originally. Closing