phetsims / balancing-act

"Balancing Act" is an educational simulation in HTML5, by PhET Interactive Simulations.

http://phet.colorado.edu/en/simulation/balancing-act

GNU General Public License v3.0

1 stars 8 forks source link

sim uses a copy of RulerNode #103

Closed pixelzoom closed 6 months ago

pixelzoom commented 4 years ago

While reviewing #79, I came across this TODO in RulerNodeRasterizedText:

* TODO: This is a copy of 'RulerNodeRasterizedText' from common code as it was on
 * 4/30/2014 with modifications - workarounds really - for an issue where the
 * text moves in undesirable ways when the ruler is rotated.  At some point,
 * the problems with rotation of text should be fixed in the browsers, at
 * which point all usages of this node can be replaces with the original
 * common-code version.  See https://github.com/phetsims/balancing-act/issues/16.

16 is closed, but these "workarounds" are still in place. Using a "copy" like this will be a problem for PhET-iO instrumentation and (eventually) a11y. It also doesn't fix the problem or allow other sims to benefit from the fix.

Assuming that "common code" is referring to SCENERY_PHET/RulerNode.... If this is really a problem, then an issue should be (and should have been) created in scenery-phet. If it's no longer an issue, then this sim should be changed to use RulerNode.

Assigning to @ariel-phet to prioritize and assign.

ariel-phet commented 4 years ago

@samreid I believe you will be doing some balancing act instrumentation, if this issue can be dealt with as part of that effort, it seems appropriate considering @pixelzoom comment above regarding iO implications.

samreid commented 4 years ago

This did not fall under the scope of the the current PhET-iO instrumentation goals, self-unassigning for now.

jbphet commented 3 years ago

I have updated the sim to use the common code version of ruler node. The motivation for the copied RulerNode was that some browsers (I'm looking at you, Firefox) were not supporting rotation of text very well when this sim was first developed. The text would tend to jump around on the ruler in a potentially distracting way as it rotated back and forth. Rasterizing the text (i.e. turning it into an image) solved the problem. Several years have now passed, and it seems like the browsers have better support for rotating text, so perhaps we can get rid of the copied version of RulerNode that had the rasterized text.

I've tested these changes on my Dell Win10 machine using Chrome, Firefox, and Edge, and on an iPad 4. In all cases, the text on the ruler looked fine, and I didn't notice any of the jumpiness and weird looking artifacts that were there years ago.

I'd like QA to test this on master using all of our supported platforms. If the text on the ruler looks okay as it moves, this can be closed. Note that we're not looking for an exact match with the currently published version, we're just verifying that the text looks decent.

KatieWoe commented 2 years ago

I believe this was addressed in https://github.com/phetsims/qa/issues/686 and determined not to be fixed. Removing the ready for qa label. @jbphet if I am mistaken and this is different let me know.

jbphet commented 2 years ago

I removed the copy of the ruler node that rasterized the text, and then had QA test it. They said that there is now some noticeable strobing of the rotating text on some platforms, see https://github.com/phetsims/qa/issues/686. It appears that the browsers that had the problem rendering rotated text before still have it, though perhaps to a lesser degree.

I'm reluctant to revert back to the copy of the ruler node because it's adding back some code duplication. I'm going to leave the code as it is, where it uses the common-code RulerNode without rasterized text, and the next time this sim is published from master we should test again and make a call whether the ruler and its rotating text are good enough without the rasterization. If we do decide to take the time to improve this, we should consider an option to RulerNode instead of having the copied and modified version that we previously had here.

marlitas commented 11 months ago

I will include this as an issue for QA to check on when we go into dev testing.

Nancy-Salpepi commented 6 months ago

With mac + chrome I see the tiniest bit of sputtering. I'm not sure I would notice this if I wasn't looking for it.

https://github.com/phetsims/balancing-act/assets/87318828/5f45df38-bf68-4b90-addf-b1b3671f8b71

KatieWoe commented 6 months ago

I also wonder if https://github.com/phetsims/balancing-act/issues/156 is somehow related.

Luisav1 commented 6 months ago

I'm also still seeing the text sputtering in Win 11 + Chrome. Not sure if it's more pronounced or just more noticeable when zoomed in. Also, the text shifts vertically when panning at higher zoom levels.

https://github.com/phetsims/balancing-act/assets/52978048/1f975e1d-8978-4862-83b0-51317135179a

@KatieWoe had a good point, though I don't think it's related to #156. Even after fixing it, the text problem remains. As well, the text 'bounces' a bit in place when zooming in.

https://github.com/phetsims/balancing-act/assets/52978048/331e70ac-0319-4a46-b01b-786a2271782f

@jbphet's earlier proposed solution involves rasterizing the text on the RulerNode by adding an option for it. As @Nancy-Salpepi said though, the text issue seems minor unless actively searched for. So, I'm leaning towards keeping the text as is without rasterization. @jbphet, could you weigh in here? Thanks!

jbphet commented 6 months ago

I just watched the video above and tested the behavior on both Firefox and Chrome on my Win 11 machine. I think the "sputtering" problem is now minor enough that we don't need to worry about rasterizing the text.

jbphet commented 6 months ago

With that, I think this issue can be closed and the republication of this sim from main can move forward.