phetsims / capacitor-lab-basics

"Capacitor Lab: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 4 forks source link

Use Shape.zigZagTo #258

Closed samreid closed 4 years ago

samreid commented 5 years ago

Capacitor Lab Basics has the following zig zag code:

    // Create the filament, which is a zig-zag shape.
    var filamentShape = new Shape().moveToPoint( filamentTopPoint );
    for ( var i = 0; i < NUM_FILAMENT_ZIG_ZAGS - 1; i++ ) {
      var yPos = filamentTopPoint.y + ( filamentBottomPoint.y - filamentTopPoint.y ) / NUM_FILAMENT_ZIG_ZAGS * ( i + 1 );
      if ( i % 2 === 0 ) {
        // zig
        filamentShape.lineTo( filamentTopPoint.x + FILAMENT_ZIG_ZAG_SPAN, yPos );
      }
      else {
        // zag
        filamentShape.lineTo( filamentTopPoint.x, yPos );
      }
    }
    filamentShape.lineToPoint( filamentBottomPoint );

Now that Shape.zigZagTo was added in https://github.com/phetsims/kite/issues/75, it may be better for this simulation to use it instead.

Assigning to @jonathanolson since he is listed as responsible dev, but also mentioning @jessegreenberg since he committed the zig-zag code.

ariel-phet commented 5 years ago

I have changed @Denz1994 to the responsible dev for this sim. Assigning to him.

@Denz1994 this does not necessarily need to be done now, but if it is straightforward might be worth polishing off.

Denz1994 commented 5 years ago

The code snippet from above has the unique property of always beginning and ending the zig-zag on a peak or trough. Kite.shape.zigZagToPoint() alternates the starting peak with an ending trough. It also starts the pattern at an amplitude of 0.

The CBL bulbs have filaments that start and end at the zig zags max amplitude. The start and end of the zig zag also always mirror each other (starts with peak, ends with peak). The lack of symmetry, in this case, gives an appearance like the images below.

With Kite.shape.zigZagToPoint() image

Current implementation: image

Denz1994 commented 5 years ago

I'll investigate the use of an offset that allows the zig zag pattern to start at some interval. My initial thought here is that a shift of lamba/2 would give the pattern desired.

Denz1994 commented 5 years ago

The addition to Shape.zigZagTo() referenced in https://github.com/phetsims/kite/issues/75#issuecomment-488122775 makes this usable in this case. I'll await input from @samreid before closing this issue as a code review is in order for this change.

Denz1994 commented 5 years ago

Labeling as on-hold until we get a solution for https://github.com/phetsims/faradays-law/issues/153

arouinfar commented 4 years ago

https://github.com/phetsims/faradays-law/issues/153 has been closed, so I'm going to remove the on-hold label.

@Denz1994 does this issue need work/review before the upcoming redeploy?

Denz1994 commented 4 years ago

It looks like this was fixed in https://github.com/phetsims/capacitor-lab-basics/commit/5a3433fa0eacc024915e8d4cda6099a6292fad23. The lightbulbs are using Shape.zigZagTo() and have the desired appearance. This issue can be closed.