diagrams / diagrams-lib

Diagrams standard library
https://diagrams.github.io/
Other
138 stars 62 forks source link

section producing incorrect results #322

Closed bacchanalia closed 5 years ago

bacchanalia commented 5 years ago

Both red sections have the same start parameters and should therefore start in the same place.

{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE TypeFamilies #-}
import Diagrams.Prelude
import Diagrams.Backend.Cairo.CmdLine

ex :: Diagram Cairo
ex = (sec 0 (0.5) <> sec (0.5) 1     # lc red) # pad 1.1
 ||| (sec 0 (0.5) <> sec (0.5) (0.8) # lc red) # pad 1.1
  where
    sec a b = strokeP $ toPath (section (circle 1) a b :: Located (Trail V2 Double))

main = mainWith ex
byorgey commented 5 years ago

One thing to note is that as you change the number 0.8 in the above code, the left endpoint of the red arc moves proportionally. I think section is simply broken. As discussed in IRC, my current thinking is that the problem is with the default implementation of section here:

https://github.com/diagrams/diagrams-lib/blob/1f863bf987c8babff112fb235ba28aa31d4c667a/src/Diagrams/Parametric.hs#L134

Transforming the parameter t1 to the parameter t1/t2 on the result of splitting at t2 assumes that parameters scale linearly. I think this may be true for individual segments, but it isn't true for trails, which give equal weight to each segment, i.e. assign parameter values in the range [i/n, (i+1)/n] to segment i if there are n total segments. If one segment gets split, this reassigns parameter values in a strange way.

fryguybob commented 5 years ago

The comments in Diagrams.Parametric seem to be explicit about the linear assumption.

bacchanalia commented 5 years ago

We have the problem that the Parametric instance of Tails depends on the number of segements, but a linear reparameterization for Sectionable can not depend on the number of segments without information about the original parameterization. One possible solution is drop the Sectionable instance for Trails and introduce a new Section type that caries that info with it.

bacchanalia commented 5 years ago

Proposal: fix the current instances so that they adhere to the property that for a <= b <= c, section t a c = section t a b <> section t b c and note in the documentation that they do not linearly reparameterize the section. Add two new types to wrap multisegment objects, I'll give them the placeholder names ProperSection and ProperParam. ProperSection still is parametric by segment, but has a linear reparameterization, and ProperParam is parametric by arclength.

byorgey commented 5 years ago

Closed by #328.