-
Notifications
You must be signed in to change notification settings - Fork 2
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
Screen doesn't pan when an object is moved with alt input #539
Comments
I beleive that this is a general issue that was reported almost 2 years ago, has been reported multiple times since, and has not been addressed. For example, see:
AFAIK, there's nothing we can do in CAV to resolved this problem. And we shouldn't be working around it in CAV; it should be addressed generally. Assigning to @jessegreenberg for comment and advice on how to proceed. |
Yes, this is a general issue we would like to improve. phetsims/scenery#1558 is where we documented the change we would like to make that would generally handle this. But it was not considered blocking at the time and not something I have been able to work on. I might get to it this iteration, but unsure. If it needs to be resolved quickly, let me know and we can discuss timelines. |
@pixelzoom - I will check in with @jonathanolson and @jessegreenberg on Monday to understand the scope of a fix and see if it can be addressed this iteration in emergent issues or is larger and needs to be scheduled in as its own interation priority. |
@kathy-phet and I discussed this issue and phetsims/scenery#1558, I will work on it this iteration and report back here when it is done. |
@jessegreenberg should we consider this blocking for an RC test, or should we move on with the expectation of a maintenance release down the line? |
IMO this is not blocking, but I don't feel like its my call. I am not planning to do a MR for this. This will be done before the end of this iteration but please check with a designer/coordinator if you think you will be ready to take RC SHAs within the next few days. |
@catherinecarter, what are your thoughts here? I think it is reasonable we will be ready to take RC SHAs be next week. Would you like us to wait for this issue to be resolved in common code before doing so? |
@marlitas - Weighing in here ... Since Jesse is working on it for this iteration, I'd like to get the fix into the sim. So @jessegreenberg, please keep the team in the loop on progress. |
Hey CaV team, I think this is resolved for this sim and you should be unblocked. To summarize: After phetsims/scenery#1558 the focused Node will generally stay in view automatically. However, that wasn't possible for the cards and the soccer balls in CaV because the "group" implementation uses a custom solution for focus management. I looked into ways scenery could find the "focused" (but not actually focused) ball/card automatically and also considered a new API for telling the pan zoom listener to "always track this Node's transform instead please", but in the end those were no better than using the AnimatedPanZoomListener directly as in the above commits. In particular please review d6cf821 and can you please review the behavior in the sim? I tried to test everything draggable to make sure it would stay in view but might have missed something. |
Good idea, I added some notes.
@marlitas you can control this with the second argument to
I tried it out but it isn't quite right. Its like
@Nancy-Salpepi this is improved in master, can you please try? |
That's tricky. That should be the right value, but it feels like there are so many interconnected things there it is very likely a delay of some sort is occurring and I'm scared to do too much without messing up other parts. I'll go in and play with it a little. I seem to prefer centering it than having it be completely off screen. I actually thought the centering was working well overall and wasn't too jarring... |
Oh! Is it because it pans before the card has animated to its new position? Is there an event when the card has finished animating? |
Ah! Yes probably. There is an this.animation.endedEmitter.addListener( () => {
this.animation = null;
this.animationTo = null;
this.animationReason = null;
} ); |
OK, thanks - here is a patch using that. I am not sure if this is the right place for this code, but it seems to work. Subject: [PATCH] Fix bug with images and other forms
---
Index: js/median/view/InteractiveCardNodeContainer.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/median/view/InteractiveCardNodeContainer.ts b/js/median/view/InteractiveCardNodeContainer.ts
--- a/js/median/view/InteractiveCardNodeContainer.ts (revision 7f5a278468d90ffded9a89989b56f4d8c7255382)
+++ b/js/median/view/InteractiveCardNodeContainer.ts (date 1695164514096)
@@ -286,6 +286,21 @@
}
};
+ // A single 'panListener' so that we can keep an easy reference to it to remove it when necessary.
+ // When using keyboard input, make sure that the "focused" card is still displayed by panning to keep it
+ // in view.
+ const panListener = () => {
+
+ // remove this listener, it will only be called once
+ if ( model.focusedCardProperty.value && model.focusedCardProperty.value.animation ) {
+ model.focusedCardProperty.value.animation.endedEmitter.removeListener( panListener );
+ }
+
+ if ( focusedCardNodeProperty.value ) {
+ animatedPanZoomSingleton.listener.panToNode( focusedCardNodeProperty.value );
+ }
+ };
+
const keyboardListener = new KeyboardListener( {
fireOnHold: true,
keys: [ 'arrowRight', 'arrowLeft', 'enter', 'space', 'home', 'end', 'escape', 'pageUp', 'pageDown' ],
@@ -344,9 +359,15 @@
model.focusedCardProperty.value = this.model.getCardsInCellOrder()[ 0 ];
}
- // When using keyboard input, make sure that the "focused" card is still displayed by panning to keep it
- // in view.
- animatedPanZoomSingleton.listener.panToNode( focusedCardNodeProperty.value );
+ assert && assert( model.focusedCardProperty.value, 'Can we assume this exists?' );
+ const focusedCard = model.focusedCardProperty.value!;
+
+ if ( focusedCard.animation ) {
+ if ( focusedCard.animation.endedEmitter.hasListener( panListener ) ) {
+ focusedCard.animation.endedEmitter.removeListener( panListener );
+ }
+ focusedCard.animation.endedEmitter.addListener( panListener );
+ }
}
}
} ); |
@jessegreenberg things are working nicely with the prediction arrows, pointer and interval tool. Panning is still a bit behind the soccer ball movement. panningWithBalls.mp4 |
@catherinecarter, your input is needed to determine wether the video Nancy showed in #539 (comment) still feels problematic. @jessegreenberg and I looked at it, and thought that having the pan move faster may be more disorienting. We can address it though if it seems like a concern. |
My initial thoughts are that the delay in catching up to focus is ok. It's a little weird that there's a slight delay in focus, but I agree, speeding it up might be disorienting. The slower pan is trackable and obvious about what's happening. While it's not distracting to me, though, I would love to interview someone using the zoomed view while moving a ball to hear what they think about the delay, though. One day, I hope to do so. |
For phetsims/qa#985, if I zoom out while a soccer ball still has focus, the ball moves to a different position. Steps:
soccerballposition.mp4 |
Ah, great find @Nancy-Salpepi thanks! I believe that is a separate issue I am going to make a new one for that. |
It seems that this issue is now be fully addressed and reviewed. Please feel free to re-open if that is not the case. Thanks all! |
Test device
MacBook Air M1 chip
Operating System
13.5.1
Browser
Safari 16.6
Problem description
For phetsims/qa#976, after zooming in, moving a soccer ball, card, prediction arrow or pointer with alt input doesn't cause the screen to pan. Instead the object just goes offscreen (Moving the marker in Geometric Optics is a good comparison).
Steps to reproduce
Here is an example:
Visuals
doesn.tpan.mp4
The text was updated successfully, but these errors were encountered: