Skip to content

line halo #870

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

line halo #870

wants to merge 3 commits into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented May 13, 2022

An unexpected difficulty was to make it compatible with group aesthetics. I had to pass a flag (in the shape of a counter) to secondary groups. For now I just "walk back to the first element", and don't use the counter per se, because we might want to switch to a real flag that would be absent from the first group—which might be more satisfying?

closes #860

a follow-up could be to add the halo option to other "non-fillable" marks: Plot.link, Plot.arrow, Plot.tick, Plot.rule…

line chart with faint halo

@Fil Fil requested review from mbostock and awiederkehr May 13, 2022 17:10
@Fil
Copy link
Contributor Author

Fil commented May 16, 2022

A problem currently is that the halo option can be an object containing anything. I don't know how to check that the values are numbers or strings as we usually do for normal options.

@Fil Fil added the question Further information is needed label May 16, 2022
@Fil Fil marked this pull request as ready for review May 16, 2022 09:34
@Fil Fil added the enhancement New feature or request label May 16, 2022
@Fil Fil marked this pull request as draft May 28, 2022 16:01
@Fil
Copy link
Contributor Author

Fil commented May 28, 2022

scratch that, a much better solution will be to use a svg filter (#409). I have a prototype here https://observablehq.com/@fil/line-halo-in-pure-svg

@Fil
Copy link
Contributor Author

Fil commented Jul 11, 2022

new version with the SVG filter strategy

@Fil Fil marked this pull request as ready for review July 11, 2022 13:06
@Fil
Copy link
Contributor Author

Fil commented Oct 18, 2022

manually rebased

@Fil Fil mentioned this pull request Feb 6, 2023
Comment on lines +9 to +15
<feMorphology in="SourceAlpha" result="DILATED" operator="dilate" radius="${radius}"></feMorphology>
<feFlood flood-color="${color}" result="BG"></feFlood>
<feComposite in="BG" in2="DILATED" operator="in" result="OUTLINE"></feComposite>
<feMerge>
<feMergeNode in="OUTLINE" />
<feMergeNode in="SourceGraphic" />
</feMerge>`);
Copy link
Member

Choose a reason for hiding this comment

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

You only need the result+in for dilated; otherwise in defaults to the previous filter primitive.

Suggested change
<feMorphology in="SourceAlpha" result="DILATED" operator="dilate" radius="${radius}"></feMorphology>
<feFlood flood-color="${color}" result="BG"></feFlood>
<feComposite in="BG" in2="DILATED" operator="in" result="OUTLINE"></feComposite>
<feMerge>
<feMergeNode in="OUTLINE" />
<feMergeNode in="SourceGraphic" />
</feMerge>`);
<feMorphology in="SourceAlpha" result="dilated" operator="dilate" radius="${radius}"></feMorphology>
<feFlood flood-color="${color}"></feFlood>
<feComposite in2="dilated" operator="in"></feComposite>
<feMerge>
<feMergeNode></feMergeNode>
<feMergeNode in="SourceGraphic"></feMergeNode>
</feMerge>`);

);

if (this.halo) {
// With variable aesthetics, we need to regroup segments by line
Copy link
Member

@mbostock mbostock Apr 1, 2023

Choose a reason for hiding this comment

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

This is hard to follow. I think it would be clearer to initialize the DOM the desired way from the beginning, rather than creating it once (above using groupIndex) and then moving things around afterwards to apply the halo. (Especially since this already requires changing groupIndex to populate a segment property on the data, whose purpose isn’t clear until you go look at the code here.)

@mbostock
Copy link
Member

mbostock commented Apr 1, 2023

This seems like a nice feature. The implementation needs some tidying.

@Fil Fil marked this pull request as draft April 3, 2023 14:42
@Fil
Copy link
Contributor Author

Fil commented Jul 31, 2023

A similar situation arises with the line opacity when the line is broken into segments for varying aesthetics: https://talk.observablehq.com/t/opacity-artifacts-in-line-segments/8142/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Halo for lines?
2 participants