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

Gradient #136

Merged
merged 29 commits into from
Apr 23, 2014
Merged

Gradient #136

merged 29 commits into from
Apr 23, 2014

Conversation

jeffreyrosenbluth
Copy link
Member

I think this and the other gradient branches are ready to merge.

@bergey
Copy link
Member

bergey commented Nov 13, 2013

I still think that users should not need to change between fc and fcT when switching backends. It's moderately annoying in code that uses gradients, and very annoying in code that only uses solid colors.

As a concrete proposal: can we have the fc function add a FillColor attribute and a FillTexture attribute? I expect that all backends will honor exactly one of those. It does change the type of fc (adding R2 constraint), but I don't expect that to break much code in the wild.

I haven't tried this. I think it provides semantics for backends that don't handle gradients, without requiring duplicate code in every backend. (I know we debated those questions in #9).

import Data.Semigroup

-- | A stop is (color, proportion, opacity)
type GradientStop = (SomeColor, Double, Double)
Copy link
Member

Choose a reason for hiding this comment

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

a) Can this be a real type instead of an alias?
b) Do we really need SomeColor? Would AlphaColor work as well?
c) Since SomeColor / AlphaColor already has opacity, do we need another double about opacity?

Copy link
Member Author

Choose a reason for hiding this comment

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

a) Do you mean data GradientStop = GradientStop SomeColor Double Double? Any reason for the preference?

@byorgey suggested b & c as well I don't have a strong preference either way. My intention was to be consistent with FillColor and LineColor.

Copy link
Member

Choose a reason for hiding this comment

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

IIUC, type doesn't provide any type safety; any (SomeColor, Double, Double) will do. Untyped Doubles make me nervous---they could reperesent anything.

I don't understand the SomeColor in FillColor either. :)

@jeffreyrosenbluth
Copy link
Member Author

@bergey re: fc and fcT - I like your suggestion , let me look at the code and see if this can work.


-----------------------------------------------------------------------------
-- |
-- Module : Diagrams.Attributes
Copy link
Member

Choose a reason for hiding this comment

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

Should be: "Module : Diagrams.TwoD.Attributes".

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, thanks

@fryguybob
Copy link
Member

You mention supporting backends that do not support gradients. Can you elaborate on what API we would end up with if we went ahead and broke all backends? Is there a clean way to have attributes that do not care what vector-space they live in work together with attributes that do? Textures appear to me to carry the meaning of functions from some space to a color (f :: Color c => R2 -> c for instance). Can we simply do the appropriate projection to move from one space to another? I know in 3D we also care about lighting but I think this can be nicely handled as a separate issue.

@jeffreyrosenbluth
Copy link
Member Author

If we broke all backends, then SomeColor would be replaced by the more general Texture which includes SomeColor, LGradient, RGradient and eventually patterns. I haven't tried to generalize Texture to arbitrary vector spaces. I got the impression from @bergey that it might not be useful for 3D? Do you think it is important?

@bergey
Copy link
Member

bergey commented Nov 13, 2013

@jeffreyrosenbluth , please correct me if I'm wrong. If we completely replace FillColor with FillTexture, all (2D) Backends need to change, but user code can stay the same---continue to use fc. Backends which do not support gradients need some way to fallback to a solid color. That's clearly better than requiring some user code to switch to fcT. But see my proposal upthread.

I thought the point of implementing gradients is that backend libraries already support them. That is, SVG can represent them directly, and cairo does something sensible (?). I agree that R2 -> color is the general form. Does that form commit us to rasterizing ourselves? I think @jeffreyrosenbluth is planning (hoping?) to implement general textures (image maps), at which point the gradient constructors become basically an optimization.

In R3 it's worse. I think POVRay (our only 3D Backend right now) only supports 2D textures surface mapped (not sure how) to the objects. That's pretty common, I think, especially for non-raytracing implementations. Parametric 3D textures are really nice, but what backend would we target? I'd love to have a native Haskell raytracer or a custom OpenGL shader, but I think either is a long way off.

@byorgey
Copy link
Member

byorgey commented Nov 13, 2013

It's been a long time since I've used POVRay, but I'm quite sure it has support for "solid", i.e. parametric 3D, textures.

@bergey
Copy link
Member

bergey commented Nov 13, 2013

@byorgey Ooh. I'll look harder in the docs, then. Thanks!

@fryguybob
Copy link
Member

Specifically I think the breaking change is replacing SomeColor in:

newtype FillColor = FillColor (Recommend (Last SomeColor))

With a Texture sum type. At which point it should be named FillTexture. As @bergey points out I don't think this changes any user code (It might change query code but we don't have that yet :D). I think this is a worthwhile change to consider. The question for me is can we generalize the vector-space nicely or not. The R2 -> Color is just the denotation for textures, so that doesn't force us to rasterize, but it does give us the semantics of what a correct implementation means.

I'm pretty sure PovRay can handle a 3d texture and not just a texture as a surface, I'll look into it.

@jeffreyrosenbluth
Copy link
Member Author

@bergey is right about the Backends needing to change if we completely replace FillColor with FillTexture and the user code can remain unchanged. But I think his proposal should be a good work around.

@fryguybob Re, generalizing from R2 to v. I'm not sure if we should try to do this now. My instinct is to get it to a point where we are happy with the 2D implementation and then think about generalizing.

@byorgey
Copy link
Member

byorgey commented Nov 14, 2013

I do like the solution of having e.g. lc apply both line color and line texture attributes. Being able to do this sort of thing is a nice benefit of having our Styles be heterogeneous collections of arbitrary attributes, of which backends only need to handle some subset (rather than e.g. a fixed sum type of attributes).

@jeffreyrosenbluth
Copy link
Member Author

If someone would put this through some tests with cairo and svg then we can add some documentation and merge.

@@ -77,6 +77,7 @@ module Diagrams.TwoD
, strokeLocTrail, strokeLocT, strokeLocLine, strokeLocLoop
, FillRule(..), fillRule
, StrokeOpts(..), vertexNames, queryFillRule
, splitFills
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for re-exporting this from Diagrams.TwoD? I think I intentionally did not re-export it because it is useful only to backend writers, who can import Diagrams.TwoD.Attributes if they need it; there's no need to export it from Diagrams.Prelude.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your right, I'll take it out

@byorgey
Copy link
Member

byorgey commented Apr 18, 2014

This is looking good to me.

data FillLoops v = FillLoops

instance Typeable v => SplitAttribute (FillLoops v) where
type AttrType (FillLoops v) = FillColor
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, I think this now needs to consider both FillColor and FillTexture, but the way I have it set up it can only handle doing one at a time. =(

One solution I suppose would be to make a second function akin to splitFills but acting on FillTexture. It's just a bit annoying to have to do two passes, but it should only be a few extra lines of code (copy lines 551-566 here and change a few things).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I don't have better answers to some of the line notes. Some of this
code was written as long as 6 months ago, so I don't remember exactly what
I was thinking at the time. I'll try to look into some the your questions
more deeply this weekend.

On Thu, Apr 17, 2014 at 10:17 PM, Brent Yorgey notifications@github.comwrote:

In src/Diagrams/TwoD/Attributes.hs:

+-- | A synonym for 'fillColor', specialized to @'Colour' Double@
+-- (i.e. opaque colors). See comment after 'fillColor' about backends.
+fc :: (HasStyle a, V a ~ R2) => Colour Double -> a -> a
+fc = fillColor
+
+-- | A synonym for 'fillColor', specialized to @'AlphaColour' Double@
+-- (i.e. colors with transparency). See comment after 'fillColor' about backends.
+fcA :: (HasStyle a, V a ~ R2) => AlphaColour Double -> a -> a
+fcA = fillColor
+------------------------------------------------------------
+
+data FillLoops v = FillLoops
+
+instance Typeable v => SplitAttribute (FillLoops v) where

  • type AttrType (FillLoops v) = FillColor

Ugh, I think this now needs to consider both FillColor and FillTexture,
but the way I have it set up it can only handle doing one at a time. =(

One solution I suppose would be to make a second function akin to
splitFills but acting on FillTexture. It's just a bit annoying to have to
do two passes, but it should only be a few extra lines of code (copy lines
551-566 here and change a few things).


Reply to this email directly or view it on GitHubhttps://github.com//pull/136/files#r11762174
.

Copy link
Member

Choose a reason for hiding this comment

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

No worries. I may have some time to help out as well.

@jeffreyrosenbluth
Copy link
Member Author

I think this gradient branches are ready to merge

@bergey
Copy link
Member

bergey commented Apr 23, 2014

+1

byorgey added a commit that referenced this pull request Apr 23, 2014
@byorgey byorgey merged commit d7f75fd into master Apr 23, 2014
@byorgey byorgey deleted the gradient branch April 23, 2014 00:51
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

4 participants