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

new attributes for separate fill & stroke opacity #265

Merged
merged 1 commit into from Oct 15, 2015

Conversation

byorgey
Copy link
Member

@byorgey byorgey commented Oct 6, 2015

See #248.

Questions I'd like help thinking about: (1) what should be the semantics of the combination of opacity, fill-opacity, and stroke-opacity attributes? (2) what is the right way to upgrade backends to support these attributes?

@byorgey
Copy link
Member Author

byorgey commented Oct 6, 2015

Travis is failing because of the thing with lens no longer exporting coerce. We need to figure out what to do about that...

@tkvogt
Copy link
Member

tkvogt commented Oct 7, 2015

I see it like this:
(1) semantics of combining opacities: opacities of one sort have to be updated the same way as other attributes: inner attributes override outer ones.
(2) In the svg backend the 3 sorts of opacities are orthogonal, and don't have to be combined, just added.
Other backends that don't know fill or stroke opacity have to draw two diagrams. stroke-opacity has to be multiplied with opacity for the first diagram and fill-opacity also has to be multiplied with opacity for the second.

@bergey
Copy link
Member

bergey commented Oct 8, 2015

db1f5c9 fixes the travis build re coerce. It's hacky, and I'm fine with some other solution, but this seemed least disruptive in the short run.

@byorgey
Copy link
Member Author

byorgey commented Oct 8, 2015

@cchalmers suggested simply not hiding coerce, which I like except for the fact that technically that requires a minor version bump, since we are now exporting something extra: the code of anyone using diagrams + an older version of lens + coerce would break. On the other hand, the number of people in that situation is likely to be approximately zero.

@byorgey
Copy link
Member Author

byorgey commented Oct 8, 2015

@tkvogt but what if you set, say, both the opacity and the fill-opacity? What should be the semantics then? Should it just ignore the opacity? Multiply them together? etc.?

@bergey
Copy link
Member

bergey commented Oct 8, 2015

That minor version bump is the reason I didn't take the unhide approach unilaterally. I just wanted the travis build to green-light. But I'm fine with that solution.

@tkvogt
Copy link
Member

tkvogt commented Oct 9, 2015

@byorgey The SVG spec says that "object/group opacity can be thought of conceptually as a postprocessing operation" (http://www.w3.org/TR/SVG/masking.html#OpacityProperty)
So stroke-opacity and fill-opacity are multiplied with opacity. If stroke-opacity, fill-opacity or opacity is not set then it has standard value 1. After having read the svg spec quite a lot I consider it quite a good guideline.

@byorgey
Copy link
Member Author

byorgey commented Oct 13, 2015

@tkvogt Thanks, that makes sense. Mostly I was trying to figure out whether e.g. setting stroke-opacity or fill-opacity should set the opacity automatically as well, but it sounds like the answer is no, they are all independent attributes and one might reasonably want to use all three of them together.

@byorgey
Copy link
Member Author

byorgey commented Oct 13, 2015

In that case I think this is ready to merge. Updating the SVG backend to use these should be easy. Updating other backends to support them sounds much harder.

bergey added a commit that referenced this pull request Oct 15, 2015
new attributes for separate fill & stroke opacity
@bergey bergey merged commit 44dfbc3 into master Oct 15, 2015
@byorgey byorgey deleted the fill-stroke-opacity branch December 17, 2015 16:19
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