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

"Data: textarea" and "State" wrappers perform poorly on iPad. #432

Closed
pixelzoom opened this issue Apr 20, 2022 · 14 comments
Closed

"Data: textarea" and "State" wrappers perform poorly on iPad. #432

pixelzoom opened this issue Apr 20, 2022 · 14 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 20, 2022

For phetsims/qa#798. @arouinfar @kathy-phet FYI.

In phetsims/qa#798 (comment) @KatieWoe reported:

Performance in the textarea and state wrappers on iPad were fairly poor.

"the textarea" is presumably the "Data: textarea" link in the Wrapper Index.

I'm surprised that these wrappers are being tested on iPad. @samreid and @zepumph can you please comment? Are these wrappers intended to be tested on iPad? Is their performance on iPad a concern?

@samreid
Copy link
Member

samreid commented Apr 22, 2022

It sounds OK for the text area and state wrappers to have poor performance on iPad. I don't recall if we requested testing those wrappers on iPad.

@samreid samreid removed their assignment Apr 22, 2022
@pixelzoom
Copy link
Contributor Author

@kathy-phet @arouinfar please decide if this issue is blocking for publication of GO 1.2. Also please provide guidance on whether QA should be testing these wrappers on iPad.

@arouinfar
Copy link
Contributor

@pixelzoom based on @samreid's comment, this is not a blocking issue.

Also please provide guidance on whether QA should be testing these wrappers on iPad.

Unless poor performance in a wrapper is indicative of poor performance of the student-facing simulation, I don't see any reason to test on iPad or Chromebook. However, this guidance should come from the PhET-iO team, so back to them.

@arouinfar arouinfar assigned samreid and unassigned kathy-phet and arouinfar Apr 25, 2022
@zepumph
Copy link
Member

zepumph commented Apr 25, 2022

I agree, they should not be blocking. These wrappers test the top level of performance capabilities of PhET-iO, with printing tons to a text area and setting state many times back to back. In the vast majority of cases these are not what are exercised in student facing wrappers (though it is possible). These are cases where we leave it up to clients to test their wrappers on the devices their students will use. You can't expect an iPad to be able to set state on a complicated sim >1 time a second and still have good usability through that process.

@zepumph zepumph assigned pixelzoom and unassigned samreid and zepumph Apr 25, 2022
@pixelzoom
Copy link
Contributor Author

... However, this guidance should come from the PhET-iO team, so back to them.

So... What is the guidance for QA? Should they be tested on iPad or not?

@zepumph
Copy link
Member

zepumph commented Apr 26, 2022

It should not. In https://github.com/phetsims/QA/blob/master/documentation/qa-book.md#test-13-data-recording-wrapper-test I see that it says:

Not supported on iPad or IE.

This is the same test, but it didn't get this documentation. @KatieWoe can you please add Not supported on iPad to test 12 also: https://github.com/phetsims/QA/blob/master/documentation/qa-book.md#test-12-data-text-area-wrapper-test

@zepumph
Copy link
Member

zepumph commented Apr 26, 2022

For the state wrapper, it is my recommendation to test on iPad. Here is what I would add as documentation for the test in the qa book:

on iPad make sure the "set state rate" is only ever 0 or >3000, this will help prevent lag.

@zepumph zepumph assigned KatieWoe and unassigned pixelzoom and zepumph Apr 26, 2022
KatieWoe added a commit to phetsims/qa that referenced this issue Apr 26, 2022
@KatieWoe
Copy link
Contributor

Done in above commit

@KatieWoe
Copy link
Contributor

As for #432 (comment), this is different from what we were previously instructed to do. Text area was specifically supported on iPad since colorized and json were not. Is this something the whole team wants changed?

@zepumph
Copy link
Member

zepumph commented Apr 27, 2022

I worry that the issue is just because of the quantity of text output, and not because of internal data stream handlings. The metacog recording handles that case by piping the data to metacog (without printing it). @samreid should we test the text area wrapper on iPad?

@samreid
Copy link
Member

samreid commented Apr 27, 2022

A spot check to make sure it runs on iPad sounds good, but text area wrapper performance does not need to be fast/smooth on iPad.

@samreid samreid assigned zepumph and unassigned samreid Apr 27, 2022
@pixelzoom
Copy link
Contributor Author

This issue is still hanging around from a dev test that was done in April 2022. And this sim is scheduled for QA and publication as part of https://github.com/orgs/phetsims/projects/44, which is likely to happen while I'm on vacation.

@zepumph @samreid is there anything to do here, or should this issue be closed?

@zepumph
Copy link
Member

zepumph commented Jul 15, 2022

Yes this issue can be closed in my opinion. We do not support the PhET-iO wrapper suite in iPad. There is though reason to think that the state wrapper should have some performance rigor attached to it, but it isn't clear how much. Perhaps we need a larger discussion to know what is acceptable for setting state. i.e. 1 per second on powerful machines, and 1 per 3 seconds for iPad.

@zepumph zepumph removed their assignment Jul 15, 2022
@pixelzoom
Copy link
Contributor Author

Closing based on @zepumph's opinion.

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

6 participants