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

section on trail producing incorrect result when upper param is >= 1 (domainUpper) #329

Closed
bacchanalia opened this issue Mar 10, 2019 · 8 comments

Comments

@bacchanalia
Copy link

Red sections should be equal to green sections.
sectionTo1

{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE UndecidableInstances #-}
{-# LANGUAGE TypeFamilies #-}
module Main where
import Diagrams.Prelude
import Diagrams.Backend.Cairo.CmdLine

ex :: Located (Trail V2 Double) -> Diagram Cairo
ex shape = foldl1 (|||) $ map (frame 5) $ [stroke shape] ++ segs 0.5 ++ segs 0.95
  where
    ε = 2**(-53) -- machine epsilon
    segs n = [base # lc green, good # lc blue, bad # lc red]
      where
        base = stroke $ section  (head $ fixTrail shape)                       n 1
        good = stroke $ section' (unfixTrail . return . head $ fixTrail shape) n (1-ε)
        bad  = stroke $ section' (unfixTrail . return . head $ fixTrail shape) n 1

ex1, ex2 :: Diagram Cairo
ex1 = ex $ vrule  100
ex2 = ex $ circle 100

dia :: Diagram Cairo
dia = (ex1 # pad 1.1) ||| (ex2 # pad 1.1)

main = mainWith (dia # bg white)

-- fix for diagrams-lib #322
class Sectionable' p where
  section' :: p -> N p -> N p -> p
instance (InSpace v n a, Fractional n, Parametric a, Sectionable' a, Codomain a ~ v) => Sectionable' (Located a) where
  section' (Loc x a) p1 p2 = Loc (x .+^ (a `atParam` p1)) (section' a p1 p2)
instance (Metric v, OrderedField n, Real n) => Sectionable' (Trail v n) where
  section' t p1 p2 = withLine (wrapLine . (\l -> section' l p1 p2)) t
instance (Metric v, OrderedField n, Real n) => Sectionable' (Trail' Line v n) where
  section' (Line t) p1 p2 = Line (section t p1 p2)
@bacchanalia
Copy link
Author

This can be fixed narrowly by changing (>=) to (>) here, but the p > 0 case is still wrong. I've eyeballed some outputs for the p < 0 and they look plausible.

@bacchanalia bacchanalia changed the title section on trail producing incorrect result when upper param is 1 (domainUpper) section on trail producing incorrect result when upper param is >= 1 (domainUpper) Mar 11, 2019
@fryguybob
Copy link
Member

The equation on line 241 looks wrong to me:

case seg `splitAtParam` (1 - (1 - p)*tSegs) of

@bacchanalia
Copy link
Author

On 243, I think the lambda should be id, since the number of segments doesn't change, but that doesn't completely fix it, so maybe an issue on 241 also.

( (SegTree $ t' |> seg1, \u -> u * tSegs / (tSegs + 1))

@bacchanalia
Copy link
Author

bacchanalia commented Mar 11, 2019

I think 241 is more naturally expressed as 1 + (p - 1)*tsegs, but I think it's correct.

I think 243 is only id when u corresponds to a previous segment, and in the case it's on the same segment it needs to be rescaled.

bacchanalia pushed a commit to bacchanalia/diagrams-lib that referenced this issue Mar 11, 2019
section on trail producing incorrect result when upper param is
>= 1 (domainUpper) diagrams#329

The first parameter was not being rescaled properly in the case that
the second parameter was >= 1. The old code tried to rescale as though
an extra segment was added, while what the code actually does is replace
the last segment with a longer one. This leads two two cases, either the
first param is on a previous segment and doesn't need to change or it's
on the last segment and has to be rescaled to the new length.
@fryguybob
Copy link
Member

Ah, yes, I was reading the code wrong. I can make sense of 241 now.

@bacchanalia
Copy link
Author

I want to rewrite sectionAtParam' for more clarity. Right now it returns ((Segtree v n, n -> n), (Segtree v n, n -> n)) but the second (n -> n) which represents the reparamaterization of the second tree is never used. Is there any reason to keep that code?

@fryguybob
Copy link
Member

The uses I can think of would be fine with sectionAtParam' only giving one remapping. However, having both could be nice for testing as you could check the consistency of computing section by comparing the results from both methods:

sectionA x t1 t2 = let ((a,fa),_) = splitAtParam' x t2 in snd $ splitAtParam a (fa t1)
sectionB x t1 t2 = let (_,(b,fb)) = splitAtParam' x t1 in fst $ splitAtParam b (fb t2)

prop_section x t1 t2 = sectionA x t1 t2 =~= sectionB x t1 t2

For some appropriate =~=.

@bacchanalia
Copy link
Author

In order to have that test we'd need to export splitAtParam', which I'm not sure is worth doing just to have that test.

byorgey added a commit that referenced this issue Mar 28, 2019
bug fix: #329 section on trail producing incorr...
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

2 participants