Skip to content
This repository has been archived by the owner on May 11, 2021. It is now read-only.

Small fix to graphie-3d -- make sure a closed path is drawn #84085

Merged
merged 1 commit into from Oct 16, 2013

Conversation

ivanistheone
Copy link
Contributor

Small fix to graphie-3d -- make sure path is drawn:

path( [v1,v2,v3] )

should be

path( [v1,v2,v3,true] ) 

to draw a closed path.

… should be path([v1,v2,v3,true]) for path to be closed path.
@beneater
Copy link
Contributor

Hey @ivanistheone can you point me to an example of where I can see this in action?

@ivanistheone
Copy link
Contributor Author

Hi Ben,

Check lines 70 to 88 in
https://ka-perseus-graphie.s3.amazonaws.com/ce84dc414be87821818c0e22d29a3c2620e1e6c2.js

Also, why use concat(true) and not push(true)?
The latter would make more sense to me.

@beneater
Copy link
Contributor

ah.. makes sense. I'll pull this into graphie-to-png and redeploy that as well.

beneater pushed a commit that referenced this pull request Oct 16, 2013
Small fix to graphie-3d -- make sure a closed path is drawn
@beneater beneater merged commit 6949a77 into Khan:master Oct 16, 2013
@ivanistheone ivanistheone deleted the upstream branch October 16, 2013 17:38
@beneater
Copy link
Contributor

should be updated on graphie-to-png now

@beneater
Copy link
Contributor

I'm gonna revert this. It's causing this to happen:

@@ -267,7 +267,7 @@ $.extend(KhanUtil, {
// draw the sketch's lines
sketch.drawLines= function() {
return graph.path(
sketch.mappedVerts(),
sketch.mappedVerts().concat(true),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the problem.
For a sketch, mappedVerts() reports the verts on the visible side --- so don't need a closed path,
just to draw some lines.

In the context of face.drawBack the concat(true) makes sense to close the path.

My mistake --- I shouldn't have changed sketch.drawLines willy-nilly just because it looks like "the same thing."

beneater added a commit that referenced this pull request Oct 21, 2013
Test plan: none

Auditors: jack
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants