Skip to content
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

Screenshot is taking too long #1139

Closed
samreid opened this issue Jan 14, 2021 · 12 comments
Closed

Screenshot is taking too long #1139

samreid opened this issue Jan 14, 2021 · 12 comments

Comments

@samreid
Copy link
Member

samreid commented Jan 14, 2021

From phetsims/circuit-construction-kit-common#646 and also related to phetsims/number-line-operations#78, we noticed it is taking 4+ seconds to generate screenshots. We compared to a published production version, and saw it was nearly instantaneous.

At today's design meeting, @kathy-phet indicated this should be a blocking-publication issue.

@jonathanolson can you please investigate?

@jonathanolson
Copy link
Contributor

It looks like it's a combination of screenshot + Chrome + filters, where Chrome's Canvas handling for named filters seems to be visually inaccurate. Notably in CCK DC screen 1:

[Contrast, Brightness]
IntroScreenView/CircuitElementToolbox/Carousel/CarouselButton/Path
[Grayscale]
IntroScreenView/CircuitElementToolbox/Carousel/CarouselButton/AlignBox
[Contrast, Brightness]
IntroScreenView/ZoomControlPanel/RectangularPushButton/Path
[Grayscale]
IntroScreenView/ZoomControlPanel/RectangularPushButton/AlignBox

If the colors can be slightly/somewhat different, I could remove the exception for Chrome here (it uses a faster path for other browsers, since those don't have the inaccuracy)

Thoughts on how important accurate colors in screenshots (or whenever we render things with Canvas) would be in Chrome? I'll try to get a visual comparison.

@jonathanolson
Copy link
Contributor

A
B

In this case, there is a very minor difference on the "diagram/symbol" button that is disabled. I have some reservations about having such a difference based on renderer (if something flickers back-and-forth in the sim between those two, it could be distracting), but it sounds potentially best to allow that. Thoughts?

@kathy-phet
Copy link

I personally cannot see a difference here. What am I looking for?

@arouinfar
Copy link

arouinfar commented Jan 15, 2021

I'm in the same boat as @kathy-phet. I don't see a difference in those two screenshots. I even downloaded them and flipped back and forth in Preview and they seem identical to me.

@arouinfar arouinfar removed their assignment Jan 15, 2021
@samreid
Copy link
Member Author

samreid commented Jan 15, 2021

When I flip back and forth, I can see the slight change in gray of the schematic radio button.

@kathy-phet
Copy link

OK - I pulled these into power point, and it looks like one color is (188,213,250) and one is (188,212,250). This is really not noticable on my computer. If you are on one computer, why would it flicker? I don't understand that?

@kathy-phet
Copy link

@jonathanolson - This is also impacting NLO. phetsims/number-line-operations#78

We want to move that sim to RC as soon as possible. How does this issue intersect with that simulation? Thanks.

@samreid samreid removed their assignment Jan 16, 2021
@jonathanolson
Copy link
Contributor

Commit above should allow the faster filter path for Chrome. @samreid can you check performance?

@jonathanolson
Copy link
Contributor

If you are on one computer, why would it flicker? I don't understand that?

It's not common, but something might switch between Canvas/SVG in certain cases, and that could show up as a visual difference (say, if you grab something from a toolbox into a play area AND they used different renderers internally). I think it's unlikely for us to run across that.

@samreid
Copy link
Member Author

samreid commented Jan 19, 2021

Performance is very speedy on my side, thanks! I also checked that the output looks reasonable. Did you intend to commit the console.log statements?

@samreid samreid assigned jonathanolson and unassigned samreid Jan 19, 2021
jonathanolson added a commit that referenced this issue Jan 19, 2021
@jonathanolson
Copy link
Contributor

Oops, fixed above. Thanks for the catch!

@samreid
Copy link
Member Author

samreid commented Jan 19, 2021

Thanks! I think this issue is ready to be closed.

@samreid samreid closed this as completed Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants