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

Particles outside of box #419

Open
ghost opened this issue Aug 1, 2019 · 9 comments
Open

Particles outside of box #419

ghost opened this issue Aug 1, 2019 · 9 comments

Comments

@ghost
Copy link

ghost commented Aug 1, 2019

Test Device

Hopper

Operating System

iOS 12.4

Browser

Safari

Problem Description

For phetsims/qa#389. I've only seen this in Wave Interference, not in Waves Intro. See the title and visuals. This bug is present in the published version.

Steps to Reproduce

  1. Interference screen.
  2. Select particles radio button or both radio button.

Visuals

IMG_0061

@ghost ghost added the type:bug label Aug 1, 2019
@ghost ghost assigned samreid Aug 1, 2019
@samreid
Copy link
Member

samreid commented Aug 2, 2019

This is a known problem from #331, but surprising that it only occurred in Wave Interference and not Waves Intro. I'll take a look.

@samreid
Copy link
Member

samreid commented Aug 2, 2019

Also, I wonder about the performance improvements @pixelzoom and @jonathanolson discussed for Gas Properties. If we can eliminate the requirement to use WebGL on iOS without sacrificing performance, we can address the clipping problem.

@samreid
Copy link
Member

samreid commented Aug 2, 2019

In the preceding commit, I made it so Waves Intro uses WebGL on mobile safari to improve performance, as we decided for Wave Interference. Good catch @lmulhall-phet! I'll move the preceding comment to #331.

@pixelzoom
Copy link
Contributor

@samreid Before investigating WebGL, you might consider making some straightforward optimizations to your for loops. I see many for loops where the exit condition is being reevaluated on every iteration. These types of optimizations resulted in a significant performance improvement (+5fps) for Gas Properties, see phetsims/gas-properties#146 (comment).

First, if your array is large, rather than using i < array.length as your exit condition, iterate in reverse. For example in IntensityGraphPanel.js:

- for ( let i = 0; i < intensityValues.length; i++ ) {
+ for ( let i = intensityValues.length - 1; i >= 0; i-- ) {

Second, if your exit condition is a constant, factor it out, do not put it in the for expression. E.g. in Lattice.js:

- for ( let i = 0; i < this.width - this.dampX * 2; i++ ) {
+ const iMax = this.width - this.dampX * 2;
+ for ( let i = 0; i < iMax; i++ ) {

@samreid
Copy link
Member

samreid commented Aug 3, 2019

I measured the profiler frame rate with throttle x4, then replaced all of the for let end conditions with constants and re-ran the profiler. There was not a significant difference in the frame rate for this simulation. I'll leave the code as it is for now.

UPDATE: To clarify, I tested both water waves (38fps) and sound particles (24fps) in requirejs mode.

@samreid
Copy link
Member

samreid commented Aug 7, 2019

In #322 we got good performance on the particles via WebGL, but did not add clipping due to the amount of time and complexity it would take to implement. This problem is noted in #331. @arouinfar or @ariel-phet do you want to work on that further before we publish?

@samreid samreid assigned ariel-phet and arouinfar and unassigned samreid Aug 7, 2019
@arouinfar
Copy link
Contributor

@samreid this is really a question of priorities and desired publication timeline. I believe the goal was to have these sims published in time for the next school year (which has already started in some parts of the country). I am fine with continuing to defer the clipping issue, but @ariel-phet should make that call.

@arouinfar arouinfar removed their assignment Aug 7, 2019
@ariel-phet
Copy link

I am fine with deferring. @samreid perhaps keep this task in mind for an intern.

@ariel-phet ariel-phet removed their assignment Aug 8, 2019
@samreid
Copy link
Member

samreid commented Aug 9, 2019

Deferring sounds great. Some notes for when we revisit this issue:

  • Implementing clipping would probably require sending clipping vertices to WebGL and would likely be done in ImageWebGLDrawable or thereabouts. It should be implemented in a way that is compatible with broader scenery clipping for the future. It would be good to get assistance or guidance from @jonathanolson.
  • For this simulation, a more expedient approach would be to "fake" the clipping by using z-order to put the particles in a back layer, then put the rest of the sim in-front-of and around them like a picture frame. The sound speaker would need to be split into left/right image parts. This strategy wouldn't be trivial or general, but would take much less time than the scenery implementation.

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