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

KickDistributionStrategy type DistanceByIndex array length bug #18

Open
marlitas opened this issue Aug 1, 2024 · 5 comments
Open

KickDistributionStrategy type DistanceByIndex array length bug #18

marlitas opened this issue Aug 1, 2024 · 5 comments

Comments

@marlitas
Copy link
Contributor

marlitas commented Aug 1, 2024

While working on phetsims/mean-share-and-balance#334 I began to wonder how the BalancePoint screen would handle an array that was the incorrect size if passed into KickDistributionStrategy as type DistanceByIndex. I discovered that it does not handle this scenario. In fact none of the soccer sims do, which means this is a bug in the published version of Center and Variability as well. I am not sure how urgent it is to fix this bug for CAV.

This is the documentation that exists:

The length of the "values" array should equal the maximum number of kicks for the scene (defined in preferences)

This does indicate that clients must use an array of the correct length for DistanceByIndex but there is no validation to prevent this, and it will break the sim if the array they provide is shorter than the max number of kicks.

What's more is this also is not taking into consideration how to handle when the number of MaxKicks changes. The documentation implies that your array should equal the max number of kicks you set in preferences, but says nothing about the fact that you need to update your array if the max number changes and you provided an array that is now too short.

Since this is a soccer-common bug the question is, does this need to be a maintenance release for CAV?

@marlitas
Copy link
Contributor Author

marlitas commented Aug 1, 2024

I talked to @amanda-phet and we think that a validation error is the best way to handle this situation. We feel like at least updating examples.md is worthy of a maintenance release.

@marlitas
Copy link
Contributor Author

marlitas commented Aug 6, 2024

Steps to reproduce for QA:

  1. Open the Balance Point Screen in Studio
  2. Go to meanShareAndBalance.balancePointScreen.model.sceneModel.kickDistributionStrategy and set the value to:
{
   "type": "distanceByIndex",
   "values": [5,9,10],
   "skewType": null
}
  1. Try to kick out all of the balls.
Screen.Recording.2024-08-06.at.11.26.29.AM.mov

marlitas added a commit to phetsims/mean-share-and-balance that referenced this issue Aug 6, 2024
marlitas added a commit to phetsims/center-and-variability that referenced this issue Aug 6, 2024
@marlitas
Copy link
Contributor Author

marlitas commented Aug 6, 2024

This is ready to cherry-pick above. @kathy-phet let me know if we would like this to be a part of a maintenance release for CAV.

@amanda-phet do you want to change anything in examples.md?

@amanda-phet
Copy link
Contributor

I don't think this warrants any changes to examples.md in MSaB, but will leave this assigned to @kathy-phet so she can weigh in on what to do for CaV (update examples.md to clarify the bug, or do a MR for CaV).

@amanda-phet amanda-phet removed their assignment Aug 12, 2024
marlitas added a commit to phetsims/mean-share-and-balance that referenced this issue Aug 13, 2024
marlitas added a commit that referenced this issue Aug 14, 2024
@Nancy-Salpepi
Copy link

For rc.2 I now see an error dialog as expected.
Screenshot 2024-08-15 at 8 38 20 AM

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

4 participants