LevelCompletedNode level

[jonathanolson]

Currently the value provided to the level field is not the level number, but 1 less than that (the level index, usually).

@samreid noted in the code review for Area Model (phetsims/area-model-common#99) that it would simplify things to have LevelCompletedNode given the actual level number instead, so that the value given is displayed.

@pixelzoom and @jbphet thoughts?

[pixelzoom]

I don't feel strongly. But I wouldn't bother changing because:

(1) Zero-based indexing in the model is the most common approach in general.

(2) It's documented correctly and clearly in LevelCompletedNode:

@param {number} level starting from zero, 1 added to this when displayed

(3) Zero-based level is used throughout vegas. E.g., FiniteStatus, line 134. If we were to change, it should be changed everywhere.

[samreid]

Here's the suspicious-looking code in area-model-common that was identified during the review, context is GameAreaScreenView (I changed the formatting to paste here)

new LevelCompletedNode( level.number - 1, ...)

I'm also wondering if we have to worry about PhET-iO naming. If it shown as "level 1" on the screen but the phet-io variable name is "level0" it could be confusing.

[pixelzoom]

The problem in area-model-common isn't with LevelCompletedNode. The problem is lack of documentation about level numbering. Apparently the model in this sim uses 1-based numbering for the levels, but doesn't document that. I looked around and could find no documentation, just redundant comments like these:

// GameAreaModel.js
    // @public {Property.<AreaLevel|null>} - The current level
    this.currentLevelProperty = new Property( null,

// AreaLevel.js
    // @public {number}
    this.number = number;

In Equality Explorer, I made this same choice, but documented it extensively. E.g.:

// implementation-notes.md
Game levels are numbered 1 to 4 in both the model and view, and in type names 
(e.g. `ChallengeGenerator1` for Level 1), so that the implementation corresponds 
to the description in the design document. This differs from the more typical approach 
of using 0-based indexing in the model, then converted to 1-based indexing in the view. 

// SolveItScene.js
* @param {number} level - game level, numbered from 1 in the model and view

[jbphet]

+1 on @pixelzoom's comments. Whether to use 0- or 1-based indexing is a decision that is always fraught with tradeoffs, and in my opinion there is never a clearly superior choice. Vegas uses 0-based, and you can change it if you want, but as @pixelzoom says it should be consistent throughout the library. It doesn't seem worth it to me, and the approach of using 1-based in the sim and documenting it clearly sounds better, but I'd be okay with it either way.

[jonathanolson]

Apparently the model in this sim uses 1-based numbering for the levels, but doesn't document that.

It's because it is labeled as / called "Level 1" on the screen and in all associated documentation about the sim and its design. Nowhere is it called "Level 0", so it makes sense for its level number to be 1.

Whether to use 0- or 1-based indexing is a decision that is always fraught with tradeoffs, and in my opinion there is never a clearly superior choice.

What is the advantage of 0-based numbers here? They are NOT indices, particularly in this case as we have "Variables" and "Numbers" levels. They are now on different screens, but there used to be two level 5s on the same screen (one "Variables" and one "Numbers"). So to me, any argument related to indexing (why 0 is usually nice) doesn't apply here.

I'm also wondering if we have to worry about PhET-iO naming. If it shown as "level 1" on the screen but the phet-io variable name is "level0" it could be confusing.

Not just confusing, but it seems like we're going out of our way to make it complicated (having to subtract 1 from level numbers to pass to VEGAS, which then has to add 1 everywhere output is required).

(3) Zero-based level is used throughout vegas. E.g., FiniteStatus, line 134. If we were to change, it should be changed everywhere.

I agree. This is something I could accomplish as part of the cleanup related to Area Model if desired?

Might be beneficial to discuss at the dev meeting, marking.

[pixelzoom]

Apparently the model in this sim uses 1-based numbering for the levels, but doesn't document that.

It's because it is labeled as / called "Level 1" on the screen and in all associated documentation about the sim and its design. Nowhere is it called "Level 0", so it makes sense for its level number to be 1.

Imo, this issue would be sufficiently addresses by simply including the above comment somewhere in the internal documentation and/or implementation-notes.md.

[jonathanolson]

Documented in the above commit in area-model. I'll be fine with either approach chosen.

[samreid]

Not just confusing, but it seems like we're going out of our way to make it complicated (having to subtract 1 from level numbers to pass to VEGAS, which then has to add 1 everywhere output is required).

That's my main consideration as well.

[jonathanolson]

@jbphet noted in today's developer meeting that it made sense to be consistent, and he will take care of the refactoring at some point.

[jbphet]

The sims updated to address this issue should be tested to make sure that the level is displayed correctly (or, in some cases, not at all). They are listed below, and I'll assign QA to check them. I spot checked several and they all looked good.

To test, please play the game at some arbitrary level (not always 1) and verify that the level value is either displayed correctly or not displayed at all. Here is the list of sims:

• build-an-atom
• graphing-lines
• fraction-matcher
• area-model
• reactants-products-and-leftovers
• arithmetic
• balancing-chemical-equations
• area-builder
• balancing-act

Here is an example of a level value that is incorrect, which I captured while making this update:

When this is done (assuming there are no problems), please assign to @jonathanolson for review.

[ghost]

@jbphet, Jacob looked at the sims in #73 (comment), and he didn't see incorrect level numbering.

I looked at all of the first levels, and I didn't see Level 0.

[pixelzoom]

From #73 (comment):

@jbphet noted in today's developer meeting that it made sense to be consistent, and he will take care of the refactoring at some point.

This doesn't tell me what you decided. What did you decide? Are we using 1-based levels throughout vegas?

[jonathanolson]

This doesn't tell me what you decided. What did you decide? Are we using 1-based levels throughout vegas?

Sorry! It was decided that we'd use the actual level numbers (1-based) throughout vegas.

[pixelzoom]

From #73 (comment):

(3) Zero-based level is used throughout vegas. E.g., FiniteStatus, line 134. If we were to change, it should be changed everywhere.

I only see changes to LevelCompletedNode. Are we changing this throughout vegas? If so, who is doing that? If not, why?

[pixelzoom]

@jbphet what is required to close this issue?

[jbphet]

I think it has been fully addressed and tested.