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

combineBoundaries isn't doing the right thing #313

Closed
byorgey opened this issue May 1, 2018 · 2 comments
Closed

combineBoundaries isn't doing the right thing #313

byorgey opened this issue May 1, 2018 · 2 comments

Comments

@byorgey
Copy link
Member

byorgey commented May 1, 2018

See https://github.com/diagrams/diagrams-lib/blob/master/src/Diagrams/Align.hs#L96 .

If we add an Alignable instance for P2 (which we should probably have anyway):

instance Alignable (Point V2 Double) where defaultBoundary = envelopeBoundary

then we should be able to align lists of points, via the Alignable [b] instance. However, this doesn't seem to do the right thing. If we call envelopeBoundary directly (which goes via the Enveloped instance for lists), it works fine:

>>> envelopeBoundary unitX (triangle 1 :: [P2 Double])
P (V2 0.5000000000000002 0.0)
>>> envelopeBoundary unit_X (triangle 1 :: [P2 Double])
P (V2 (-0.4999999999999999) 0.0)

But if we call defaultBoundary, which goes via the Alignable [b] instance and hence the combineBoundaries function, we get this:

>>> defaultBoundary unitX (triangle 1 :: [P2 Double])
P (V2 0.5000000000000002 0.0)
>>> defaultBoundary unit_X (triangle 1 :: [P2 Double])
P (V2 0.5000000000000002 (-0.0))

Notice we get the same boundary point no matter which direction we query.

@byorgey
Copy link
Member Author

byorgey commented May 1, 2018

combineBoundaries
  :: (InSpace v n a, Metric v, Ord n, F.Foldable f)
  => (v n -> a -> Point v n) -> v n -> f a -> Point v n
combineBoundaries b v fa
= b v $ F.maximumBy (comparing (quadrance . (.-. origin) . b v)) fa

Comparing the resulting points by their absolute distance from the origin makes no sense. We want the point which is furthest in the direction of v, so we should be comparing them by projecting onto v and comparing the (signed) magnitude of the results.

@byorgey
Copy link
Member Author

byorgey commented May 1, 2018

I think this is correct but I am not 100% sure.

combineBoundaries b v fa
  = b v $ F.maximumBy (comparing (dot v . (.-.origin) . b v)) fa

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

No branches or pull requests

1 participant