Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
amanda-phet opened this issue Sep 14, 2016 · 28 comments

Comments

@amanda-phet
Copy link
Contributor

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
Copy link
Contributor

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 pixelzoom assigned amanda-phet and unassigned pixelzoom Sep 14, 2016
@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 14, 2016

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
Copy link
Contributor

pixelzoom commented Sep 14, 2016

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.

pixelzoom added a commit that referenced this issue Sep 14, 2016
@amanda-phet
Copy link
Contributor Author

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
Copy link

@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

@ariel-phet ariel-phet changed the title Can we change the rows slider to go down to 1? Consider allowing minimum rows to go to "1" and addressing depenency on current minimum of "5" Sep 14, 2016
@amanda-phet
Copy link
Contributor Author

Sounds good to me.

Unassigning.

@amanda-phet amanda-phet removed their assignment Sep 14, 2016
@ariel-phet
Copy link

@pixelzoom thanks for the prompt investigation

@pixelzoom pixelzoom changed the title Consider allowing minimum rows to go to "1" and addressing depenency on current minimum of "5" allow minimum rows to go to "1" and address dependency on current minimum of "5" Sep 14, 2016
@kathy-phet
Copy link

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
Copy link
Contributor

@kathy-phet said:

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

This issue will suffice.

@kathy-phet
Copy link

ok

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 15, 2016

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]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants