phetsims / density-buoyancy-common

Common code for the Density and Buoyancy simulations
GNU General Public License v3.0
0 stars 2 forks source link

block at min density will bounce indefinitely on Mercury #404

Closed Nancy-Salpepi closed 21 hours ago

Nancy-Salpepi commented 3 weeks ago

Test device MacBook Air M1 chip

Operating System 14.6.1

Browser Chrome

Problem description For https://github.com/phetsims/qa/issues/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

https://github.com/user-attachments/assets/8b5c4195-833b-4143-b5f1-2c3cb4584ee1

KatieWoe commented 2 weeks ago

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

DianaTavares commented 2 weeks ago

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

buoyancy

zepumph commented 5 days ago
  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 commented 5 days ago

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

zepumph commented 5 days ago

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).

Nancy-Salpepi commented 2 days ago

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

KatieWoe commented 2 days ago

I was able to get this again in https://github.com/phetsims/qa/issues/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

Nancy-Salpepi commented 2 days ago

nice find Katie!

zepumph commented 1 day ago

I could see us doing nothing about it and just closing the issue. I could also see us trying to work out some other solution. We already decided in other places that trying to adjust how the physics behaves was too challenging and nerve racking this late in the publication, and as a result we increased the min density so that this kind of thing wasn't possible. I believe double checking with @DianaTavares is best here. My thought would be to close this.

kathy-phet commented 1 day ago

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

samreid commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

@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.

KatieWoe commented 21 hours ago

Looks good!