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

Console errors when dragging Custom block to bottom with alt input #403

Closed
Nancy-Salpepi opened this issue Sep 14, 2024 · 13 comments
Closed
Assignees
Labels
dev:alt-input 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, dragging a block at min density and min volume to the bottom of the pool causes an error which makes the block temporarily unmovable with alt input. Tabbing away and then back again fixes the problem.

Steps to reproduce

  1. In Basics on the Explore Screen, Select "Custom"
  2. Move the Density and Volume sliders to minimum values
  3. Tab to and grab the block
  4. Drag the block to the bottom of the pool
  5. Once the block pops back up, try to move it with alt input

Visuals

minDensity.mp4
Screenshot 2024-09-14 at 4 24 14 PM
@Nancy-Salpepi Nancy-Salpepi added the type:bug Something isn't working label Sep 14, 2024
@Nancy-Salpepi
Copy link
Author

When the block is at the minimum density but not at minimum volume (ex. 3.5 or 4.5), the block will continuously sputter when dragged to the bottom of the pool with alt input.

minDensity2.mp4

@Nancy-Salpepi
Copy link
Author

Also, when using the pointer, I can drag the block through the scale when it has the minimum density and volume values.

blockThroughScale.mp4

@Nancy-Salpepi
Copy link
Author

Following the steps in #403 (comment) in Studio shows this:
Screenshot 2024-09-19 at 9 00 30 AM

@zepumph zepumph added dev:help-wanted Extra attention is needed dev:alt-input labels Oct 1, 2024
@zepumph
Copy link
Member

zepumph commented Oct 1, 2024

Maybe related to other work we were doing with the pivot/constraint logic in #367, but I thought we reverted that.

@samreid
Copy link
Member

samreid commented Oct 3, 2024

This problem is easy to reproduce. Dragging the low density, small block to the bottom of the pool triggers this code which was added in #377

      // If the mass is under the pool (likely because user forced it there), interrupt dragging and move it back up.
      if ( !( mass instanceof Scale ) && mass.matrix.m12() < this.poolBounds.minY ) {
        mass.teleportUp();
      }

which interrupts the dragging and removes the pointer constraint. So we have options like:

  1. Prevent teleportation in cases like this
  2. Prevent teleportation from interrupting input at all
  3. Make sure teleportation fully interrupts input so that the next drag would be clean and start over with a new pointer constraint.
  4. Prevent keyboard drag from trying to push the block too far into the ground

@samreid
Copy link
Member

samreid commented Oct 3, 2024

This is incredibly buggy, but only for a very low density, low volume block. Try dragging the block with the mouse, it can go through the ground easily. Likely p2 is having trouble.

@samreid
Copy link
Member

samreid commented Oct 3, 2024

These numbers address the bugs for a variety of mass + volume combinations in buoyancy:basics explore:

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/PhysicsEngine.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/PhysicsEngine.ts b/js/common/model/PhysicsEngine.ts
--- a/js/common/model/PhysicsEngine.ts	(revision c22a525940eb520b74ef717f986c16ae83ab1183)
+++ b/js/common/model/PhysicsEngine.ts	(date 1727965539272)
@@ -379,10 +379,12 @@
 
     body.wakeUp();
 
+    const maxForce = DensityBuoyancyCommonQueryParameters.p2PointerMassForce * body.mass + DensityBuoyancyCommonQueryParameters.p2PointerBaseForce;
+    console.log( DensityBuoyancyCommonQueryParameters.p2PointerMassForce, body.mass, DensityBuoyancyCommonQueryParameters.p2PointerBaseForce, maxForce );
     const pointerConstraint = new p2.RevoluteConstraint( nullBody, body, {
       localPivotA: globalPoint,
       localPivotB: localPoint,
-      maxForce: DensityBuoyancyCommonQueryParameters.p2PointerMassForce * body.mass + DensityBuoyancyCommonQueryParameters.p2PointerBaseForce
+      maxForce: maxForce
     } );
     this.pointerConstraintMap[ body.id ] = pointerConstraint;
     this.world.addConstraint( pointerConstraint );
Index: js/common/DensityBuoyancyCommonQueryParameters.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/DensityBuoyancyCommonQueryParameters.ts b/js/common/DensityBuoyancyCommonQueryParameters.ts
--- a/js/common/DensityBuoyancyCommonQueryParameters.ts	(revision c22a525940eb520b74ef717f986c16ae83ab1183)
+++ b/js/common/DensityBuoyancyCommonQueryParameters.ts	(date 1727965619016)
@@ -110,14 +110,14 @@
   // An amount of force applied to all pointer-mass interactions (e.g. when you grab it with the mouse)
   p2PointerBaseForce: {
     type: 'number',
-    defaultValue: 2500
+    defaultValue: 10
   },
 
   // Adds to the amount of force applied to all pointer-mass interactions (e.g. when you grab it with the mouse), but
   // is this value TIMES the mass's mass value.
   p2PointerMassForce: {
     type: 'number',
-    defaultValue: 0
+    defaultValue: 250
   },
 
   // Stiffness:

How can we ensure that we don't disrupt previously working functionality with a force change like this?

@samreid samreid removed the dev:help-wanted Extra attention is needed label Oct 3, 2024
@zepumph
Copy link
Member

zepumph commented Oct 3, 2024

Does the solution in #404 completely solve this bug?

@zepumph zepumph assigned samreid and unassigned zepumph Oct 3, 2024
@zepumph
Copy link
Member

zepumph commented Oct 4, 2024

Taking a look

@zepumph zepumph assigned zepumph and unassigned samreid Oct 4, 2024
@zepumph
Copy link
Member

zepumph commented Oct 4, 2024

I tested after fixing #404 by increasing the minimum density allowed for the custom block in B:B:Explore and can no longer reproduce. @Nancy-Salpepi let me know if you can though (in next RC).

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

This is mostly fixed in rc.2.
If at min density and min volume, dragging the block to the bottom with shift + down arrow will still produce sputtering (like in #403 (comment)) .

@Nancy-Salpepi
Copy link
Author

Nancy-Salpepi commented Oct 7, 2024

Sorry--I was only looking at the PhET brand sim earlier.
I can still reproduce the sputtering noted in #403 (comment) if I change the density of a mystery block to a small value in Studio (eg. density = 15 and fluid density = 12)

@samreid
Copy link
Member

samreid commented Oct 7, 2024

@zepumph and I reproduced the sputtering in a few cases. We feel it is part of p2, and we are glad there is no TypeError on the console to go along with it. Since @Nancy-Salpepi did not report the TypeError, this issue seems safe to close.

@samreid samreid closed this as completed Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev:alt-input type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants