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

Vertices #192

Merged
merged 5 commits into from Jun 5, 2014
Merged

Vertices #192

merged 5 commits into from Jun 5, 2014

Conversation

jeffreyrosenbluth
Copy link
Member

*** Please do NOT merge ***

  • I'd like to freeze HEAD until 1.2 is uploaded.
  • I want to see what others think of this definition of Vertex

The idea is that Trails and Paths only have vertices in between segments if the derivative (slope) changes.
That way for example a hexagon has 6 vertices, but a circle has none. Unlike the current implementation where a circle has 4 vertices. The an objects vertices should not depend on its implementation, see the trello card about getting rid of decorateFoo. I think that the semantics implemented here makes sense since segments (lines and beziers) are always differentialbe, the only place we can have a vertex is at a connection, hence it suffices to check the slopes at the connections.

@bergey
Copy link
Member

bergey commented Jun 3, 2014

This seems like a useful function. How about adding a primed variant with a user-specified tolerance?

I've been trying to think when I've used the current version, and whether there's a reason to have both. I sometimes use it to illustrate where segments are, for debugging or documentation. It's not too hard to go through expandTrail, but if other people also do this, maybe it's worth keeping the old version under some other name. Thoughts? Or maybe we should put various illustrateTrail functions in -contrib?

@jeffreyrosenbluth
Copy link
Member Author

Yes I would like to add a primed version.

I'm not so sure about leaving the a version of the existing implementation. I'm worried about what happens if for example we change the way we make circles to use 6 curves instead of 4. Not only would this be a difficult bug for users to find, it would not necessarily be simple for them to fix having relied on the 4 equidistant verices.

It might be useful to have various illustrateTrail vesions in contrib but, i still think we should have a decorateTrail in lib.

@jeffreyrosenbluth
Copy link
Member Author

It turns out I needed to reinstate the old functions under the names trailPoints, segmentPoints, linePoints and loopPoints since there were several places in -lib that relied on these functions.

@bergey
Copy link
Member

bergey commented Jun 3, 2014

If you're keeping functions with the old behavior, I think they should have the old names, and add the new behavior with a new name. Otherwise user code changes behavior without any compiler error / warning.

@byorgey
Copy link
Member

byorgey commented Jun 3, 2014

But the point is that we want to discourage users from using the old functions, right? How are the old functions used in -lib? Can we work around it somehow?

@jeffreyrosenbluth
Copy link
Member Author

It's not that simple, some of the old uses require the old names but some require the new semantics, like decoarateTrail so either way some behavior will change.
@byorgey , Perhaps the best thing to do is not export the old functions from the Prelude.

@byorgey
Copy link
Member

byorgey commented Jun 3, 2014

Ah, yes, not exporting them from the Prelude sounds like exactly the right thing to do.

@jeffreyrosenbluth
Copy link
Member Author

Is there an easy way to export all of a module except a handful of functions? Or do i need to enumerate all of the other functions in Trail.hs? Maybe if I import Trail.hs hiding ...

@byorgey
Copy link
Member

byorgey commented Jun 3, 2014

Hmm, not really. Either that, or put the non-exported functions in a sub-module like Trail.Internal and don't re-export them from Trail.

@jeffreyrosenbluth
Copy link
Member Author

I think this should be ready to merge.

bergey added a commit that referenced this pull request Jun 5, 2014
@bergey bergey merged commit 84a3f48 into master Jun 5, 2014
@bergey bergey deleted the vertices branch June 5, 2014 00:50
trailVertices' toler (viewLoc -> (p,t))
= withTrail (lineVertices' toler . (`at` p)) (loopVertices' toler . (`at` p)) t

-- : Like trailVertices' but the tolerance is set to tolerance
Copy link
Member

Choose a reason for hiding this comment

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

The syntax for this comment needs to be fixed.

@byorgey
Copy link
Member

byorgey commented Jun 5, 2014

I am happy to see this got merged since I think it is definitely the right direction. However, the fact that we have to resort to approximate comparisons of slopes up to some tolerance makes me squeamish. It means that e.g. vertices can appear or disappear when you perform a nonuniform scale. I wonder about a more intensional approach, where the location of vertices is part of the semantics of a trail, and has to be specified up front (with some obvious defaults). This would also allow one to place "vertices" at locations other than sharp corners.

More generally I think there is still room for thinking more deeply about the semantics of trails. @fryguybob and I have talked about it a few times. One of the long-term goals, I think, is that segments should become entirely an implementation detail which are invisible to the user. Right now segments are mixed up in the semantics of trails in a funny way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants