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

Speed of ray animation #71

Closed
sarchang opened this issue Jul 2, 2021 · 10 comments
Closed

Speed of ray animation #71

sarchang opened this issue Jul 2, 2021 · 10 comments

Comments

@sarchang
Copy link
Contributor

sarchang commented Jul 2, 2021

How fast should the rays be drawn?

@sarchang sarchang self-assigned this Jul 2, 2021
@arouinfar arouinfar self-assigned this Jul 6, 2021
@veillette
Copy link
Contributor

@arouinfar will play with the simulation.
The constant that control the speed of the rays is in GeometricOpticsConstants

  HORIZONTAL_SPEED: 5, // (speed in meter per second for light)

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 15, 2021

GeometricOpticsConstants .HORIZONTAL_SPEED no longer exists. It appears to have been replaced by GeometricOpticsConstants.ANIMATION_TIME.

But I've tried changing ANIMATION_TIME, and I see no difference in the amount of time that the rays animation takes.

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 15, 2021

I've renamed the constant and documented it better. It's now GeometricOpticsConstants.RAYS_ANIMATION_TIME. But I still don't see any change when I increase the value.

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 15, 2021

There's something very wrong with how RAYS_ANIMATION_TIME is being used. If I change it to a smaller value, then instead of the animation going faster, the animation stops before the rays have been fully rendered.

For example, here's RAYS_ANIMATION_TIME: 0.1:

screenshot_1243

And here's RAYS_ANIMATION_TIME: 0.5:

screenshot_1244

@pixelzoom pixelzoom assigned pixelzoom and unassigned arouinfar Sep 15, 2021
@pixelzoom pixelzoom added type:bug Something isn't working and removed design:polish labels Sep 15, 2021
@veillette
Copy link
Contributor

@pixelzoom: There are two constants that govern the animation of the rays.

One is the speed of the rays and the other it the animation time. The product of them gives the distance travelled.

  LIGHT_SPEED: 500, // speed in centimeter per second for light for the purpose of the animation
  ANIMATION_TIME: 5, // maximum animation time

The distance travelled should be at least larger than the width of the screen.

The two constants, as defined, are probably not as orthogonal as they should be since changing the animation time to a small value, one is forced to increase the speed value in order to have the rays span the screen.

@veillette
Copy link
Contributor

It should be possible to have a less heuristic approach.
The stopping of the ray animation could be determined by a leftward horizontal position, which when reached stopped the animation.

I'll also mention that one could not set any maximum animation time and let the animation go on forever. It didn't seem to have any performance effects on our laptops but I suspect this is not the case on all devices.

@pixelzoom
Copy link
Contributor

@veillette thank you for the explanation, that all makes sense.

In the above commit, I increased RAY_ANIMATION_TIME to 10 seconds, and I replaced LIGHT_SPEED with this lightSpeed query parameter, so that designers can experiment:

  // speed of light in centimeters per second, for the purpose of the light rays animation
  lightSpeed: {
    type: 'number',
    defaultValue: 500,
    isValidValue: value => ( value >= 100 )
  }

@arouinfar please review, let me know if you'd like to change the default (500) to something else. Note the constraint that lightSpeed must be >= 100. That seems plenty slow. Any slower and you risk running into the problem that I described in #71 (comment).

@pixelzoom pixelzoom assigned arouinfar and unassigned pixelzoom Sep 22, 2021
@pixelzoom pixelzoom added status:ready-for-review and removed type:bug Something isn't working labels Sep 22, 2021
@arouinfar
Copy link
Contributor

The behavior in master feels pretty good to me and is sufficient for the prototype.

@ariel-phet can you test out lightSpeed to see if there would be a better value?

@arouinfar arouinfar assigned ariel-phet and unassigned arouinfar Oct 1, 2021
@ariel-phet
Copy link

ariel-phet commented Oct 3, 2021

@arouinfar @pixelzoom I have a slight preference for lightSpeed to be equal to 400, 500 feels a shade too fast if the goal is to kind of comfortably have one's eye trace the path of the light rays. But my preference is minor.

If 400 still feels good to you @pixelzoom go with it. If you prefer 500, stick with that.

@ariel-phet ariel-phet assigned pixelzoom and unassigned ariel-phet Oct 3, 2021
pixelzoom added a commit that referenced this issue Oct 5, 2021
@pixelzoom
Copy link
Contributor

I like @ariel-phet's suggestion here. Slightly slower (400) feels good to me too. I have made it so in the above commit. Closing.

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

5 participants