Closed pixelzoom closed 2 years ago
Thanks for making the issue, I agree this should not stop publication releases at the moment, and while I think we should investigate this issue and fix it in the long run, this issue seems like something that can wait a bit since its consequences are pretty minor. I've added it to quarterly planning to discuss if we do it in Q1 2022.
RewardNode uses CanvasNode and sets canvasBounds
for the content when initialized and when the ScreenView transform changes. In the case of pan/zoom an ancestor of the ScreenView has a transform that changes so updateBounds
is never called.
We can use TransformTracker to watch transforms along the Trail to the RewardNode. This means using getUniqueTrail
to get the Trail to the RewardNode, disallowing DAG. But RewardNode already uses getUniqueTrail
to find the ScreenView to attach the listener to, so this has always been a limitation for RewardNode. Ill give this a try.
This is potentially something that shouldn't require too much work, and was marked as a goal for Q1 2022. Removing the deferred label.
The change to use TransformTracker to watch transforms of any ancestor (not just the ScreenView) to update RewardNode bounds was done in the above commit.
I tested this by zooming in and out of the RewardScreenView and also by verifying the case in https://github.com/phetsims/equality-explorer/issues/176 was fixed (RewardNode looks correct when it starts animating while already zoomed in).
EDIT: An easier way to test this case is to add a conditional to RewardScreenView.step so you can control from the dev tools when the RewardNode starts animating and zoom in first.
step( timeElapsed ) {
if ( window.running ) {
this.rewardNode.step( timeElapsed );
}
}
@samreid since you were the original author of RewardNode and previously worked on canvasDisplayBounds
can you please review this change? Feel free to pass back to me if you don't have time at the moment and Ill solicit another reviewer.
Working copy patch for the review:
At today's quarterly planning meeting, it was requested to increase the priority of this review. Maybe get to it within a few weeks?
I tested zooming in before enabling the reward node. I tried eroding the bounds of the reward node, so I could see the edge. Everything seemed to behave as expected.
From the code inspection:
updateBounds
can be converted from a class instance variable to a temporary closure variable, like const updateBounds = ...
Everything else seems good, nice work. Back to @jessegreenberg
Great ideas, thanks. I added a note of doc and an assertion to make sure that the trail to the RewardNode hasn't changed between updateBounds
calls. Thanks for reviewing, back to you @samreid.
I tested the assertion with this patch in RewardScreenView.ts:
Looks great, thanks! Closing.
The assertion added in https://github.com/phetsims/vegas/commit/efe7863e2835c3adfe5c40193e9fb22b2bc9eca2 is causing a new failure in CT. See https://github.com/phetsims/vegas/issues/111.
The general zoom feature is not working properly with RewardNode. RewardNode does not adjust to fill the visibile bounds of the ScreenView. See for example https://github.com/phetsims/equality-explorer/issues/176. And this seems to be happening with all sims that use RewardNode.
I believe @jessegreenberg was the developer for the zoom feature, so I'll start by assigning to him. We'll start with high prioirity, because this impacts the RC testing for Equality Explorer 1.1, phetsims/qa#735. If you'd like to verify whether this needs to be fixed for Equality Explorer 1.1 (and possibly adjust the priority), please consult with @amanda-phet.