-
Notifications
You must be signed in to change notification settings - Fork 6
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
Mystery Screen: Random block set has inappropriate volume range #154
Comments
It seems like this issue is limited to just the "Random" set of blocks. The other sets on the Mystery screen have the appropriate volume range. I've updated the issue title to reflect the actual problem. |
I added the range constraints above to the random masses, but this only triggers an internal assertion (I guess that isn't detected by phet-io?) Presumably we'd have to do something a bit crazy, like dynamically adjusting the range of the massProperty based on the customDensityProperty, and the customDensityProperty based on the massProperty in order to enforce the volume constraint. Is that type of thing desired? |
Thanks @jonathanolson. Every block now has an appropriate
That is so strange! I wonder why PhET-iO was previously able to detect the assertion, as in #135 (comment). PhET-iO common code has changed significantly since then, so it's possible that it's just the new normal rather than an actual regression.
No need to do anything crazy. The examples doc already cautions clients to make sure that the resultant volume will be in range when updating the However, it might be worth checking in with @samreid or @zepumph for a second opinion just in case, though. @jonathanolson can you reach out? |
@zepumph, thoughts on whether it's possible to detect a downstream assertion and notify in studio? (If we change the density, then the mass, it could put it outside of the computed "volume" range. Ideally we could detect that, instead of having to update ranges on any changes, which would be brittle). |
Let's talk when the time and priority arises, I don't think async is best for something like this. |
As far as Density goes, this is a high priority @zepumph. It would be nice to understand why Studio can no longer detect the out-of-range value downstream. If this is just the expected behavior due to other changes in PhET-iO common code, that's fine. However, if this is unexpected, I think it deserves some investigation to figure out why the behavior has changed. |
I launched Density | Studio and followed the first step:
It resulted in this error dialog in studio: @arouinfar what else needs to be done for this issue? |
@jonathanolson and I worked on it, and added a custom validator. We have a much nicer error message like this: Hoping to commit soon. |
…ng if it would result in an out-of-range volume, see phetsims/density#154
…ng if it would result in an out-of-range volume, see phetsims/density#154
@jonathanolson and I added the guard I described above. We tested it well in the Mystery Screen on the massProperty. @arouinfar can you please test? |
@samreid @jonathanolson everything looks good is master, thanks! For future reference, this is what the warning currently looks like: |
For #150
On the Mystery Screen, we allow clients to directly set the
massProperty
of the blocks. When the mass is changed in this way, the volume of the block automatically changes to maintain a constant density. The examples doc cautions clients to make sure that the resultant volume is within range (1-10 L). However, there is no enforcement of this within Studio, but there used to be.In #135 (comment) I detailed an example that would result in an out-of-range volume and there was previously an error in Studio, so this looks like a regression.
customDensityProperty
to 100000 kg/m^3.massProperty
to 1000 kg.The text was updated successfully, but these errors were encountered: