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

block at min density will bounce indefinitely on Mercury #404

Open
Nancy-Salpepi opened this issue Sep 14, 2024 · 20 comments
Open

block at min density will bounce indefinitely on Mercury #404

Nancy-Salpepi opened this issue Sep 14, 2024 · 20 comments
Assignees
Labels
priority:2-high type:bug Something isn't working

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air M1 chip

Operating System
14.6.1

Browser
Chrome

Problem description
For phetsims/qa#1141 in Buoyancy Basics on the Explore Screen, a block with a density of .01 kg/L will continually bounce on Mercury.

Steps to reproduce

  1. In Basics, go to the Explore Screen and move the Density slider to the minimum value
  2. Change the Fluid to Mercury

Visuals

Bouncing.mp4
@Nancy-Salpepi Nancy-Salpepi added the type:bug Something isn't working label Sep 14, 2024
@KatieWoe
Copy link

Some of the other materials/densities have odd behavior too. Saltwater doesn't bounce at all and it looks a bit odd.
morefunk

@DianaTavares
Copy link

I also get these behaviors, same: low-density block and high-density fluid:

buoyancy

@zepumph
Copy link
Member

zepumph commented Oct 3, 2024

  1. In buoyancy, when minCustomMass was .1, it should be .5 instead
  2. in Buoyancy basics, min custom density should be .1 instead of .01.
Subject: [PATCH] Node from the vbox needs to be set to invisible, https://github.com/phetsims/density-buoyancy-common/issues/415
---
Index: js/common/model/Material.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Material.ts b/js/common/model/Material.ts
--- a/js/common/model/Material.ts	(revision 3bc283c608f96c20bada618bf0c1ceeb07646d9f)
+++ b/js/common/model/Material.ts	(date 1727982771091)
@@ -404,7 +404,7 @@
     nameProperty: DensityBuoyancyCommonStrings.material.materialTStringProperty,
     hidden: true,
     colorProperty: DensityBuoyancyCommonColors.materialTColorProperty,
-    density: 950 // Same as the Human's average density
+    density: Material.HUMAN.density
   } );
 
   public static readonly MATERIAL_U = new Material( packageJSON.name === 'buoyancy' ? SOLIDS_TANDEM.createTandem( 'materialU' ) : Tandem.OPT_OUT, {
Index: js/buoyancy/view/BuoyancyLabScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/view/BuoyancyLabScreenView.ts b/js/buoyancy/view/BuoyancyLabScreenView.ts
--- a/js/buoyancy/view/BuoyancyLabScreenView.ts	(revision 3bc283c608f96c20bada618bf0c1ceeb07646d9f)
+++ b/js/buoyancy/view/BuoyancyLabScreenView.ts	(date 1727982535089)
@@ -116,7 +116,7 @@
         visiblePropertyOptions: {
           phetioFeatured: true
         },
-        minCustomMass: 0.1,
+        minCustomMass: 0.5,
         maxVolumeLiters: maxBlockVolume
       }
     ) ] );
Index: js/buoyancy/view/BuoyancyExploreScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/view/BuoyancyExploreScreenView.ts b/js/buoyancy/view/BuoyancyExploreScreenView.ts
--- a/js/buoyancy/view/BuoyancyExploreScreenView.ts	(revision 3bc283c608f96c20bada618bf0c1ceeb07646d9f)
+++ b/js/buoyancy/view/BuoyancyExploreScreenView.ts	(date 1727982535098)
@@ -51,7 +51,7 @@
       model.blockB,
       this.popupLayer, {
         tandem: tandem,
-        minCustomMass: 0.1
+        minCustomMass: 0.5
       }
     );
 
Index: js/buoyancy-basics/view/BuoyancyBasicsExploreScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy-basics/view/BuoyancyBasicsExploreScreenView.ts b/js/buoyancy-basics/view/BuoyancyBasicsExploreScreenView.ts
--- a/js/buoyancy-basics/view/BuoyancyBasicsExploreScreenView.ts	(revision 3bc283c608f96c20bada618bf0c1ceeb07646d9f)
+++ b/js/buoyancy-basics/view/BuoyancyBasicsExploreScreenView.ts	(date 1727982535080)
@@ -55,7 +55,7 @@
         ownsCustomDensityRange: false,
         customKeepsConstantDensity: true,
         tandem: tandem,
-        minCustomMass: 0.1,
+        minCustomMass: 0.5,
         maxCustomMass: 15
       }
     );

@zepumph
Copy link
Member

zepumph commented Oct 4, 2024

working on this now since I hope it is simple and easy.

@zepumph zepumph assigned zepumph and unassigned samreid Oct 4, 2024
zepumph added a commit that referenced this issue Oct 4, 2024
zepumph added a commit that referenced this issue Oct 4, 2024
zepumph added a commit that referenced this issue Oct 4, 2024
zepumph added a commit that referenced this issue Oct 4, 2024
zepumph added a commit that referenced this issue Oct 4, 2024
@zepumph
Copy link
Member

zepumph commented Oct 4, 2024

Alright, we increased the min custom density in B:B:Explore, and increased the min custom mass in B:Lab and B:Explore. QA, please do some testing around the changes, for example in studio, to make sure we didn't cause any issues in certain cases where the mass or density is derived to be a (now) unsupported value).

@zepumph zepumph removed their assignment Oct 4, 2024
@Nancy-Salpepi
Copy link
Author

I no longer get the endless bouncing in rc.2.
Closing.

@KatieWoe KatieWoe reopened this Oct 7, 2024
@KatieWoe
Copy link

KatieWoe commented Oct 7, 2024

I was able to get this again in phetsims/qa#1149. If you fill the bottle in the last screen of the Density sim fully with air it has a density of .01 kg/L and will bounce infinitely on a max density fluid.
bouncebottle

@samreid samreid changed the title block at min density will bounce indefinately on Mercury block at min density will bounce indefinitely on Mercury Oct 7, 2024
@kathy-phet
Copy link

I agree with MK that this is a corner case, and we should close as won't fix.

@samreid
Copy link
Member

samreid commented Oct 7, 2024

This is caused by error accumulation in the physics engine. We can address this problem by reducing the p2 timestep from the current value of 1 / 120 to about 1 / 540. However, this is a very risky change that will impact behavior everywhere and have more significant performance implications. Therefore, we do not recommend changing the time step.

@zepumph and I found that we can change the bottle mass from 0.1kg to 0.3kg and that is enough to prevent the bouncing. This is much more well-isolated and less risky solution, if we want to do anything. Furthermore, when we faced similar bouncing problems, we solved it by adjusting the min/max densities to reduce the occurrence of bouncing. So adjusting the bottle mass would be in line with our prior decisions.

@DianaTavares do you see any problems with changing the bottle mass from 0.1kg (current value) to 0.3kg (proposed value that fixes the bouncing)?

@Nancy-Salpepi
Copy link
Author

Also in studio, if I customize the density material of the mystery block I can get it to bounce indefinitely. (eg. a density of 0.8 will cause the block to keep bouncing on mercury). I think designers will realize not to use this value, but wanted to note that this can happen.

@zepumph
Copy link
Member

zepumph commented Oct 8, 2024

@DianaTavares @samreid and I like increasing the mass of the empty bottle to .5kg. As for studio, we are not worried about that since all PhET-iO studio customizations involve a certain level of design and testing.

zepumph added a commit that referenced this issue Oct 8, 2024
zepumph added a commit that referenced this issue Oct 8, 2024
zepumph added a commit that referenced this issue Oct 8, 2024
zepumph added a commit that referenced this issue Oct 8, 2024
@KatieWoe
Copy link

KatieWoe commented Oct 8, 2024

Looks good!

@KatieWoe KatieWoe closed this as completed Oct 8, 2024
@samreid samreid reopened this Oct 10, 2024
@zepumph
Copy link
Member

zepumph commented Oct 10, 2024

From slack:

Kathy Perkins
:spiral_calendar_pad: Yesterday at 19:42
Hey Buoyancy team,
Congratulations on the publication!! So much hard work and an amazing sim! The teachers will LOVE it!
I wanted to follow-up on one of the last changes that was made and just make sure the team realizes on the ramifications of it and agrees that its the preferred of the two options - and that is the increase of the plastic bottle weight from 0.1 to 0.5 kg.
#404
We fiddled with that plastic bottle a lot in design, and we have it as a taking up "0 volume" on its own ... so now it is a 0.5kg and 0L object. You can fill the object with 10L of 3.15 kg/L concrete and see that the bottle displaces 10L of water. The "system" density used to be 3.16kg/L ... just a tad more, due to the mass we had assigned to our "thin plastic", but now the system density is affected a lot more by mass of the bottle - bumping it to 3.20kg/L. And its an even bigger difference when we fill with less dense material ... Gasoline goes from 0.68 to 0.73 kg/L instead of just to 0.69 kg/L. I want to make sure this was realized ... since for this screen it seems like a bigger deal pedagogically than other places where the minimum mass was increased to 0.5 kg.
What does the team think? (My inclination is that the bouncing corner case is OK here, and the 0.1kg bottle is better/preferred for this context so it impacts the System density less. We could revert to 0.1kg this before we publicize.)
Please don't let this question to detract from the major accomplishment! I'm very excited for us!
Kathy (edited)

3 replies

Diana Lopez
:spiral_calendar_pad: 21 minutes ago
I didn't think about the changes that you mentioned Kathy. You are right, it is a huge change now in the system density. And now i also think is OK the strange unlimited jumping when the bottle is empty and the fluid is super dense... is possible to go back to 0.1 kg? Sorry for not to think before well about this.

Sam Reid
14 minutes ago
It is straightforward to change, just a few busywork steps, so would be good to confirm the value before changing it. We saw little or no bouncing at 0.3kg (in the long run), and took 0.5kg just to be safe, but could just go back to 0.1kg. What do you recommend?

Diana Lopez
:spiral_calendar_pad: 9 minutes ago
I think that 0.1kg is better, no matter the edges where it bounce.
@kathy
, what do you think?

So let's proceed with reverting the bottle mass change, and doing a Maintenance release.

@kathy-phet
Copy link

I agree, the smaller the better - so 0.1kg sounds ok to me.

@zepumph
Copy link
Member

zepumph commented Oct 10, 2024

Fixing now

zepumph added a commit that referenced this issue Oct 10, 2024
zepumph added a commit that referenced this issue Oct 10, 2024
zepumph added a commit that referenced this issue Oct 10, 2024
zepumph added a commit that referenced this issue Oct 10, 2024
@zepumph
Copy link
Member

zepumph commented Oct 10, 2024

Ready for rc + local spot check + production deploy for MR.

@zepumph
Copy link
Member

zepumph commented Oct 10, 2024

@kathy-phet
Copy link

@zepumph - We did not advertise anything about PhET-iO, so hoping even if this changed the API, you can just proceed and overwrite it, without worrying about migration anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:2-high type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants