search

phetsims / kite

A library for creating, manipulating and displaying 2D shapes in JavaScript.
MIT License
12 stars 6 forks source link

Add Shape.zigZagTo #75

Closed samreid closed 5 years ago

samreid commented 5 years ago

I added zig-zag code for the filament for the fuse in Circuit Construction Kit, in https://github.com/phetsims/circuit-construction-kit-common/issues/479 and https://github.com/phetsims/circuit-construction-kit-common/issues/469.

image

I asked on Slack if the zig-zag code should be moved to common code, general agreement was that it should be moved. We also identified several other places with zig-zagging code:

Here's the full conversation for archival purposes:

Sam Reid [10:07 AM]
I added a zig zag shape command for CCK.  I’m wondering if it should remain in CCK or be moved to Shape.js.
https://github.com/phetsims/circuit-construction-kit-common/blob/1c809c55b6e27491b896efe77c53505a6a05379d/js/view/zigZag.js
Pasted image at 2019-04-22, 10:07 AM 

Michael Kauzmann [10:17 AM]
My vote would be to move to Shape.js

Jesse Greenberg [10:18 AM]
Could be useful for other sims with circuits, faradays-law also has a zig zag

Sam Reid [10:22 AM]
Thanks, I see the FL zig-zag code now.  I’m also seeing something similar in ExpressionNode and capacitor lab basics bulb filament.

Jonathan Olson [10:22 AM]
I'm fine with it in Shape.js
it seems atypical for methods on a Shape to include the start point (since it already has that information)
what do you think of if it was moveTo( ... ).zigZagTo( ... ) where the zig-zag didn't include the start?

Sam Reid [10:23 AM]
That seems right to me
samreid commented 5 years ago

I moved this implementation from Circuit Construction Kit Common, and ported CCK to use it. I opened new issues for code that looks like it should use this new implementation. @jonathanolson can you please review?

Denz1994 commented 5 years ago

@samreid Sims that require a symmetrical zig zag don't get drawn properly (see https://github.com/phetsims/capacitor-lab-basics/issues/258#issuecomment-486489352 and https://github.com/phetsims/faradays-law/issues/153#issuecomment-487789685).

I added a flag to determine which drawing pattern to use ( asymmetrical/symmetrical). It seems a bit hackish to have two drawing patterns in the same function, so your input on fixing this bug would be helpful.

veillette commented 5 years ago

The previous commit handles zig-zags for no-vertical orientation using the same API. Assigning to @jonathanolson to review.

samreid commented 5 years ago

Unassigning since @jonathanolson is requested for review, but let me know if I should be reassigned.

jonathanolson commented 5 years ago

Looks good to me!

samreid commented 5 years ago

Thanks @jonathanolson. This issue seems good and ready to close, but note there is still outstanding work to be done in https://github.com/phetsims/faradays-law/issues/153#issuecomment-498742921