search

phetsims / plinko-probability

"Plinko Probability" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
5 stars 7 forks source link

allow minimum rows to go to "1" and address dependency on current minimum of "5" #84

Open amanda-phet opened 7 years ago

amanda-phet commented 7 years ago

One of our collaborators who observed sims in the classroom this summer had the suggestion to allow the board to go down to a single peg. I agree with him that it would really emphasize the concept of binary probability, and would allow teachers to more flexibly teach this concept (when they are usually teaching it with coins that don't have an adjustable probability).

Is the code structured in a way that allows you to change the number of rows easily? I understand that a single row would probably scale the peg to be very large, but it seems worth it.

pixelzoom commented 7 years ago

I'm sorry to report that very bad things happen if I change the minimum number of rows to 1. Some examples:

First, the size of the pegs and placement of their shadows is mess up. Here's what the "Intro" screen looks like at startup ("Lab" screen is similar):

screenshot_188

Here's what the "Lab" screen looks like when rows is set to 1. Both the peg and its shadow are not properly positioned, and (I believe) the peg should be larger:

screenshot_189

Next is the issue of ball size. With rows=1, here's what you get when the Lab screen is dispensing continuous balls:

screenshot_195

I poked around, and there's no easy fix for any of these problems. And some of them (like setting a maximum ball size) would require a design meeting or two to work out.

@amanda-phet How do you want to proceed?

pixelzoom commented 7 years ago

If you want this change, I'd estimate somewhere in the neighborhood of 20-30 hours to ferret out all of the problems, decide how to address them, and adjust the implementation. And we should stop dev testing immediately.

pixelzoom commented 7 years ago

Btw... The problems with the size and location of pegs and their shadows occurs for any number of rows !== 5. So there's something in the implementation (multiple somethings, actually) that are dependent on the value '5'. This is a bad thing, and would be nontrivial to fix. I'll add a note in the code so that someone doesn't step on this landmine.

amanda-phet commented 7 years ago

OK, that is a bummer... it is probably not worth 20-30 hours at this point!

@ariel-phet @kathy-phet do you agree?

ariel-phet commented 7 years ago

@amanda-phet I don't think this would be appropriate at this time considering @pixelzoom's schedule

However, having the code written in a way that depends on the minimum value of 5 is problematic and we should address. I think this would be something we could consider for Jesse once he is done Pendulum Lab (which is next for him after he finishes GAO). He appreciates "smallish" projects.

At worse case, we could have Martin and his team rework things next summer for a 2.0 release

amanda-phet commented 7 years ago

Sounds good to me.

Unassigning.

ariel-phet commented 7 years ago

@pixelzoom thanks for the prompt investigation

kathy-phet commented 7 years ago

Thanks for investigating. And I agree - defer, but we should probably log a future issue too, to refine the code regardless. Jesse sounds like a good candidate at some point if it seems good to him.

pixelzoom commented 7 years ago

@kathy-phet said:

we should probably log a future issue too, to refine the code regardless.

This issue will suffice.

kathy-phet commented 7 years ago

ok

pixelzoom commented 7 years ago

9/15/16 dev meeting: Make a note in the code review checklist to monkey around constants during review, to check for this kind of issue. Target constants that are likely candidates for being changed in the future.

[EDIT: Done, see https://github.com/phetsims/phet-info/issues/29]

amanda-phet commented 7 years ago

Quick ping- here is the issue that @pixelzoom estimated 20-30 hours to address. This sim will be the next priority the math grant.

pixelzoom commented 7 years ago

@ariel-phet @amanda-phet I reviewed this issue again, and must retract my statement made earlier today that fixing this would probably be "a quickie". Looks like it's going to be more involved, and I won't know exactly how much time until I'm well into fixing it.

veillette commented 7 years ago

I'll take a look at it.

veillette commented 7 years ago

Setting the minimum number of rows to 1, for screen intro

image

whereas for the lab screen image

veillette commented 7 years ago

and for the record here is what it looks for one row

image

veillette commented 7 years ago

if you set the minimum number of Rows to 1 and set the default value to 3, here is what the intro tab looks like: image

veillette commented 7 years ago

The two previous commits only addressed the second part of this issue, namely the dependency on the number of rows being equal to five. I think you would need a design meeting to decide on the best scaling for the balls/ cylinders/pegs/hopper when the number of rows is small.

veillette commented 7 years ago

There are a lot of small details that become more prevalent when the number of rows get small. (1) the peg shadow does not look realistic when the number of rows is equal to 1 (2) the diameter of the balls is larger than the hopper (4) in the intro tab, the balls at the bottom of the cylinder get clipped (5) the cylinders get filled up very quickly (6) for n=1 the balls seems to go through the pegs.

veillette commented 7 years ago

The clipping of the balls in the cylinder is due to the canvasBounds of the balls that is determined by 

    var ballViewBounds = this.modelViewTransform.modelToViewBounds( ballModelBounds ).dilated( 20 );

the 20 should be increased and set to be proportional to the radius of the largest ball.

ariel-phet commented 7 years ago

@veillette it seems that the scaling shown in https://github.com/phetsims/plinko-probability/issues/84#issuecomment-294163728 looks good. Could we try a rule the for rows being 3 or less the scaling is fixed at what you have shown? As in size of pegs and balls?

veillette commented 7 years ago

I added query parameters minRow, maxRow and defaultRow so you can see for yourself. eg. plinko-probability_en.html?minRow=1&defaultRow=3 will set the minimum number of Row to 1

Below are some screenshots. FYI, I only changed the size of the balls, which is relatively trivial to do. Changing the size of the balls in such arbitrary manner affects the intro screen if the default value is set below three (by default it is 12 so it is not affected by the change). You will see that the balls do not stack neatly in the cylinder bins.

Changing the size of the pegs is easy but the balls do not bounce off the pegs anymore since the pegs and the balls are no longer bound by the proper scaling. In other words, it would take a bit more effort to change the size of the pegs.

image

image

veillette commented 7 years ago

If you are serious about setting n=1 and having it worked nicely so that the balls seem to bounced off the pegs (rather than just arbitrarily setting the radius of the balls and the pegs) , it would be preferable to work on a branch rather than master.

ariel-phet commented 7 years ago

@veillette yes, we are serious about that feel free to continue in a branch

pixelzoom commented 7 years ago

[4/19/17, 11:41:24 AM] Ariel Paul: Hey Chris, Martin said he is happy to do the Plinko work, and has some time over the next few weeks to work away at it

Assigning to @veillette.

veillette commented 7 years ago

I addressed the issue of scaling the pegs and the balls for the number of rows 1 to 4 in #99.

Closing

samreid commented 4 years ago

There are still TODOs marked for this issue, discovered during https://github.com/phetsims/chipper/issues/946

samreid commented 4 years ago

Unassigning until we return focus to this sim.