phetsims / hookes-law

"Hooke's Law" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 4 forks source link

Springs missing from screenshots #87

Closed Nancy-Salpepi closed 2 months ago

Nancy-Salpepi commented 2 months ago

Test device MacBook Air M1 chip

Operating System 14.5

Browser Safari 17.5 and Chrome

Problem description For https://github.com/phetsims/qa/issues/1102, on all screens the springs don't show up when I take a screenshot from the PhET menu. This doesn't happen in published.

Steps to reproduce Use the Screenshot from the PhET menu to take a screenshot of any screen.

Visuals Hooke's Law screenshot Hooke's Law screenshot-2 Hooke's Law screenshot-3

pixelzoom commented 2 months ago

Spomething like this happened recently in https://github.com/phetsims/faradays-electromagnetic-lab/issues/116, where electrons were missing from screenshots. In that case, electrons were implemented using scenery Sprites, and the canvasBounds was missing.

In this case, the springs are instances of HookesLawSpringNode, which is a subclass of scenery-phet's ParametricSpringNode, which is a subclass of Node.

@jonathanolson do you have any thoughts on what might be causing this, and how to correct?

pixelzoom commented 2 months ago

Reproduced on MacBook Pro Intel with macOS 14.5 and Chrome 126.0.6478.127.

I do not see the problem with masses-and-springs, another sim that uses ParametricSpringNode.

pixelzoom commented 2 months ago

The culprit is this line in HookesLawSpringNode:

      boundsMethod: 'none' // method used to compute bounds for phet.scenery.Path components, see Path.boundsMethod

In https://github.com/phetsims/hookes-law/issues/3#issuecomment-121154006, @jonathanolson recommended this to improve performance:

My first proposed change is to not compute the bounds of the coil (use boundsMethod: 'none'), and my guess is that would have about a 35% speedup in execution time over boundsMethod: 'unstroked', which itself seemed to have been at least 2x faster than boundsMethod: 'accurate' (the default and latest dev version). For layout, instead of using the bounds, compute how big the spring will be.

@jonathanolson thoughts on why this is now a problem for the Screenshot feature?

pixelzoom commented 2 months ago

I discussed with @jonathanolson. The implementation of the Screenshot feature has changed much since Hooke's Law 1.0 was branched in 2015, and is likely incompatible with boundsMethod: 'none'. The best course forward is probably to remove boundsMethod: 'none', and test on devices that may be performance challenged.

pixelzoom commented 2 months ago

I removed boundsMethod: 'none' from HookesLawSpringNode. We are now using boundsMethod: 'accurate', the default in ParametricSpringNode. I published 1.1.0-dev.19 for testing. Performance feels great on my MacBook Pro and iPad6.

@Nancy-Salpepi please test on some performance-challenged devices. Identify the devices and how you would describe the responsiveness/smoothness of the user experience while updating the springs.

Nancy-Salpepi commented 2 months ago

I used my lowest performing device, a Lenovo 100e Chromebook, and saw no issues with the springs. There was no delay when moving the sliders/handle and the expansion/contraction of the springs was smooth. I saw no differences in performance between dev.19 and published.

Also the springs are now in the screenshots!

Are we good to close this @pixelzoom?

pixelzoom commented 2 months ago

Excellent, thanks @Nancy-Salpepi. Closing.