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

feat: allow LinePlot to be drawn in steps #99

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions lib/chart/lineplot.ex
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ defmodule Contex.LinePlot do
custom_y_formatter: nil,
width: 100,
height: 100,
smoothed: true,
stroke_width: "2",
colour_palette: :default
]
Expand Down Expand Up @@ -106,7 +105,9 @@ defmodule Contex.LinePlot do

- `:stroke_width` : 2 (default) - stroke width of the line

- `:smoothed` : true (default) or false - draw the lines smoothed
- `:plot_style` : :direct (default), :smooth or :step

For backwards compatibility, the option :smoothed was kept and is translated: true -> :smooth and false -> :direct.

Note that the smoothing algorithm is a cardinal spline with tension = 0.3.
You may get strange effects (e.g. loops / backtracks) in certain circumstances, e.g.
Expand Down Expand Up @@ -254,7 +255,10 @@ defmodule Contex.LinePlot do
y_accessor,
colour
) do
smooth = get_option(plot, :smoothed)
# keep backwards compatibility with old `:smoothed` option
style = get_option(plot, :plot_style) |> IO.inspect(label: "style")
smoothed = get_option(plot, :smoothed) |> IO.inspect(label: "smoothed")
plot_style = if smoothed != nil, do: smoothed, else: style
Copy link
Owner

@mindok mindok Aug 14, 2024

Choose a reason for hiding this comment

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

Should this be:

plot_style = if smoothed != nil, do: :smooth, else: style

(i.e. :smoothed atom set)

Copy link
Owner

Choose a reason for hiding this comment

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

Ah - I see what you've done - the true/false is handled in the path logic. I think it's better to definitively set the plot_style here for a couple of reasons:

  1. We isolate the deprecation logic to a single spot
  2. The paths code may be used from elsewhere in the future and the options (true, false, :smooth, :direct, :step) are overlapping and unclear
  3. There are fewer places to go to remove the deprecated code

so something like:

  plot_style = case smooth do
    true -> if smoothed, do: :smooth, else: :direct
    _ -> style
  end

Copy link
Author

Choose a reason for hiding this comment

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

Adjusted to

    plot_style =
      case smoothed do
        true -> :smooth
        false -> :direct
        _ -> style
      end

stroke_width = get_option(plot, :stroke_width)

options = [
Expand Down Expand Up @@ -282,7 +286,7 @@ defmodule Contex.LinePlot do
|> Enum.chunk_by(fn {_x, y} -> is_nil(y) end)
|> Enum.filter(fn [{_x, y} | _] -> not is_nil(y) end)

Enum.map(points_list, fn points -> line(points, smooth, options) end)
Enum.map(points_list, fn points -> line(points, plot_style, options) end)
end

@doc false
Expand Down
31 changes: 27 additions & 4 deletions lib/chart/svg.ex
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,10 @@ defmodule Contex.SVG do
]
end

def line(points, smoothed, opts \\ []) do
def line(points, plot_style, opts \\ []) do
attrs = opts_to_attrs(opts)

path = path(points, smoothed)
path = path(points, plot_style)

[
"<path d=\"",
Expand All @@ -85,7 +85,11 @@ defmodule Contex.SVG do

defp path([], _), do: ""

defp path(points, false) do
defp path(points, nil), do: path(points, :smooth)
defp path(points, true), do: path(points, IO.inspect(:smooth))
Copy link
Owner

Choose a reason for hiding this comment

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

Would be good to remove the calls to IO.inspect() once we're happy with the changes.

Copy link
Author

Choose a reason for hiding this comment

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

Removed the whole distinction on the function level and moved the mapping to the calling function, as you suggested above.

defp path(points, false), do: path(points, IO.inspect(:direct))

defp path(points, :direct) do
Enum.reduce(points, :first, fn {x, y}, acc ->
coord = ~s|#{x} #{y}|

Expand All @@ -94,9 +98,28 @@ defmodule Contex.SVG do
_ -> [acc, [" L ", coord]]
end
end)
|> IO.inspect()
end

defp path(points, :step) do
Enum.reduce(points, :first, fn {x, y}, acc ->
coord = ~s|#{x} #{y}|

case acc do
:first ->
["M ", coord]

_ ->
previous_coord = acc |> List.last()
previous_y = previous_coord |> String.split(" ") |> List.last()
new_x = x
acc ++ [" L ", ~s|#{new_x} #{previous_y}|, " L ", coord]
end
end)
|> IO.inspect()
end

defp path(points, true) do
defp path(points, :smooth) do
# Use Catmull-Rom curve - see http://schepers.cc/getting-to-the-point
# First point stays as-is. Subsequent points are draw using SVG cubic-spline
# where control points are calculated as follows:
Expand Down
8 changes: 6 additions & 2 deletions test/contex_line_chart_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,12 @@ defmodule ContexLineChartTest do
C330.6666666666667,237.5 388.3333333333333,60 404,20 "
"""

output = Contex.SVG.line(points, true) |> IO.iodata_to_binary()
IO.inspect(output)
output_new = Contex.SVG.line(points, :smooth) |> IO.iodata_to_binary()
output_old = Contex.SVG.line(points, true) |> IO.iodata_to_binary()

assert output_new == output_old

IO.inspect(output_new)
end
end
end
Loading