Make it possible to disable dragging the laser, while still being able to turn the button on and off

samreid commented 3 years ago

In https://github.com/phetsims/scenery-phet/issues/660#issuecomment-772618956 @arouinfar said:

For PhET-iO we are going to want to be able to make the body of the lasers pickable:false while keeping the button interactive. The same issue came up in pH Scale, see phetsims/scenery#1041 and phetsims/scenery#1116. I don't know if that has any impact on the current issue, but I want to make you aware of this design requirement in case it does.

Note that a solution/workaround was proposed in https://github.com/phetsims/scenery/issues/1041#issuecomment-772685077

samreid commented 3 years ago

At today's design meeting, @arouinfar and @kathy-phet also commented that we should be able to disable drag/rotate independently.

zepumph commented 3 years ago

Ideally any solution in https://github.com/phetsims/scenery/issues/1041 wouldn't involve working on deprecated listeners. Is part of the Bendling Light work to upgrade LaserNode's SimpleDragHandlers to DragListener?

samreid commented 3 years ago

We discussed that maybe removing the knob is how you make it non-rotateable.

samreid commented 3 years ago

I added isRotationEnabledProperty and isTranslationEnabledProperty in laserNode and they have the correct behavior in both screens. isTranslationEnabledProperty has no effect on the "intro" and "more tools" screen where the laser cannot be translated. @arouinfar and @pixelzoom does this seem reasonable to you? Please keep in mind the context of https://github.com/phetsims/scenery/issues/1041 and how this pattern could be applied more generally.

pixelzoom commented 3 years ago

Yep, this is what I had in mind (in bending-light, and more generally for phetsims/scenery#1041). I played with bending-light in Studio, and (to me) this method of enabling/disabling interaction feels superior to setting pickable, or (worse) having to find and disable listeners that are relevant to some interaction that you want to turn off.

Some recommendations:

phetioDocumentation (and code comments) for these new Properties seems like a must-have. Especially since this is a different approach than instructional designers (and PhET developers) have seen in other sims.
In Studio, I see both laserNode.isRotationEnabledProperty and laserNode.isTranslationEnabledProperty in all 3 screens. But isRotationEnabledProperty does nothing in the Prisms screen, and isTranslationEnabledProperty does nothing in the Intro and More Tools screens. This was confusing to me at first, and I suspect it will also be confusing to instructional designers. Recommended to instrument these Properties only where they are relevant -- ie, only where the interaction is actually possible.

samreid commented 3 years ago

isRotationEnabledProperty does nothing in the Prisms screen

On the prisms screen, disabling isRotationEnabledProperty hides the rotation knob on the back of the laser and makes it non-rotatable.

pixelzoom commented 3 years ago

Ah, I missed that on the Prisms screen. But you get the point, right? isTranslationEnabledProperty is being instrumented (and therefore presented in Studio) in cases where there is no "translate" interaction.

samreid commented 3 years ago

Yes, I'm working on removing it from that case.

pixelzoom commented 3 years ago

This would address my concerns:

Index: js/common/view/LaserNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/LaserNode.js b/js/common/view/LaserNode.js
--- a/js/common/view/LaserNode.js   (revision a5b9562b142dddb1ba0299c3e1c8383e60b4695f)
+++ b/js/common/view/LaserNode.js   (date 1612925642727)
@@ -215,19 +215,21 @@
       }
     } );

-    const isTranslationEnabledProperty = new BooleanProperty( true, {
-      tandem: options.tandem.createTandem( 'isTranslationEnabledProperty' )
-    } );
-    isTranslationEnabledProperty.lazyLink( isEnabled => {
-      if ( isEnabled ) {
-        translationTarget && translationTarget.addInputListener( translationListener );
-        translationTarget && translationTarget.addInputListener( translationOverListener );
-      }
-      else {
-        translationTarget && translationTarget.removeInputListener( translationListener );
-        translationTarget && translationTarget.removeInputListener( translationOverListener );
-      }
-    } );
+    if ( translationTarget ) {
+      const isTranslationEnabledProperty = new BooleanProperty( true, {
+        tandem: options.tandem.createTandem( 'isTranslationEnabledProperty' )
+      } );
+      isTranslationEnabledProperty.lazyLink( isEnabled => {
+        if ( isEnabled ) {
+          translationTarget.addInputListener( translationListener );
+          translationTarget.addInputListener( translationOverListener );
+        }
+        else {
+          translationTarget.removeInputListener( translationListener );
+          translationTarget.removeInputListener( translationOverListener );
+        }
+      } );
+    }

     // update the laser position
     laser.emissionPointProperty.link( newEmissionPoint => {

pixelzoom commented 3 years ago

Side issues...

These lines seem unfortunate. Why even create these listeners if there is no translatonTarget?

143     translationTarget && translationTarget.addInputListener( translationListener );
154     translationTarget && translationTarget.addInputListener( translationOverListener );

This is also mega confusing, since knobImage might actually be null, and it's not documented very well:

    const translationTarget = hasKnob ? laserPointerNode : knobImage;
    const rotationTarget = hasKnob ? knobImage : laserPointerNode;

Somehow we're supposed to figure out what the knob is used for. The above code makes it look like the knob is sometimes used for translation, but I'm pretty sure (but not certain) that it never is, and knobImage is null in that case. This would be a step in the right direction:

    const translationTarget = hasKnob ? laserPointerNode : null;

... and I still don't understand the semantics of hasKnob. It also means that the laser can't be translated?... So overloaded.

samreid commented 3 years ago

I think I addressed all of the recommendations in the commits, can you please review at your convenience?

pixelzoom commented 3 years ago

Changes looks good, and address my concerns about isTranslationEnabledProperty.

hasKnob is still way too overloaded for my taste. I've figured out that it means:

true - add a knob, make the laser rotate by dragging the knob, translate by dragging the laser body
false - don't add a knob, make the laser rotate by dragging the laser body, don't support translation

If it were me, I would (1) document that LaserNode is always rotatable, (2) always show the knob, since there's no harm in doing so and imo it's confusing that it's removed, and (3) replace hasKnob with isTranslatable:

@param {boolean} isTranslatable
  true: the laser can be translated by dragging it by the laser body, rotated by dragging it by the knob
  false: the laser cannot be translated, and can be rotated by dragging anywhere on the laser

... but up to you whether you want to do anything about hasKnob.

kathy-phet commented 3 years ago

FYI, amy and I identified several more applications of this issue in our projectile motion review this week, so MK you will get to try any approach there also.

Sent from my iPhone

On Feb 9, 2021, at 8:20 PM, Chris Malley notifications@github.com wrote:

Changes looks good, and address my concerns about isTranslationEnabledProperty.

hasKnob is still way too overloaded for my taste. I've figured out that it means:

true - add a knob, make the laser rotate by dragging the knob, translate by dragging the laser body false - don't add a knob, make the laser rotate by dragging the laser body, don't support translation If it were me, I would (1) document that LaserNode is always rotatable, (2) always show the knob, since there's no harm in doing so and imo it's confusing that it's removed, and (3) replace hasKnob with isTranslatable:

@param {boolean} isTranslatable true: the laser can be translated by dragging it by the laser body, rotated by dragging it by the knob false: the laser cannot be translated, and can be rotated by dragging anywhere on the laser — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

samreid commented 3 years ago

Tagging @zepumph to make sure he is aware of the progress in this issue and the preceding comment about Projectile Motion. I will move the recommendations from https://github.com/phetsims/bending-light/issues/397#issuecomment-776410279 to a separate issue, closing.

arouinfar commented 3 years ago

FYI, amy and I identified several more applications of this issue in our projectile motion review this week, so MK you will get to try any approach there also.

@zepumph the cannon in Projectile Motion can be adjusted in two ways:

dragging the barrel to change the angle
dragging the cannon base/pedestal to change the height

Clients should be able to independently disable these interactions (whether by pickableProperty or something else). I'll include this request in the issue for the Projectile Motion design review.

phetsims / bending-light

Make it possible to disable dragging the laser, while still being able to turn the button on and off #397