Advanced Flexbox Layout #259

siefkenj · 2021-05-26T03:22:53Z

Here's some initial work towards ideas pointed out in #258

Most of the work so far is in nodes.rs, which builds the layout and interfaces with the stretch library. I am now working on ChartLayout which should be similar to ChartBuilder but with additional flexibility. However, I could use some pointers...

Something with how I am doing error handling in ChartLayout isn't right. In examples/layout.rs, if I use ? to unwrap the errors, I get a compile error:

returns a value referencing data owned by the current function

which I assume is because the error is somehow related to the DrawingArea to which I hold a mutable reference. However, I tried to mirror your code closely, so I'm not sure what's wrong. (If I use .unwrap() instead of ? it works.) Any ideas?

codecov · 2021-05-26T03:27:32Z

Codecov Report

Merging #259 (8cc45ea) into master (b054a80) will increase coverage by 2.39%.
The diff coverage is 82.55%.

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
+ Coverage   68.53%   70.92%   +2.39%     
==========================================
  Files          61       64       +3     
  Lines        4322     5204     +882     
==========================================
+ Hits         2962     3691     +729     
- Misses       1360     1513     +153

Impacted Files	Coverage Δ
src/chart/builder.rs	`75.78% <ø> (ø)`
src/coord/mod.rs	`50.00% <ø> (ø)`
src/coord/ranged1d/combinators/logarithmic.rs	`58.33% <ø> (ø)`
src/coord/ranged1d/mod.rs	`92.59% <ø> (ø)`
src/coord/ranged2d/cartesian.rs	`78.26% <ø> (ø)`
src/drawing/area.rs	`91.58% <0.00%> (-1.43%)`	⬇️
src/style/palette.rs	`0.00% <ø> (ø)`
src/style/text.rs	`30.66% <ø> (ø)`
src/chart/layout/mod.rs	`75.34% <75.34%> (ø)`
src/element/basic_shapes.rs	`94.44% <88.88%> (-0.43%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b054a80...8cc45ea. Read the comment docs.

siefkenj · 2021-05-26T16:47:59Z

I think I figured it out...I switch to concrete error types instead of dyn std::error::Error and now it works. I don't know why the dyn Error was causing issues, though...

siefkenj · 2021-05-27T18:51:48Z

@38 The bulk of the API should be implemented now. Please let me know what you think.

One question I had is about the LayoutError enum. I modified it so that it could encapsulate errors for ChartLayout. It doesn't look like LayoutError is used anywhere in plotters or any of the plotters backends. Does anyone use this? Is it a breaking change?

So far I have implemented labels and extents, but not yet the passdown to the sub-region (chart_area) where plotting actually takes place. I plan to do something similar to ChartBuilder and just pass back a drawing area restricted to that location. The only thing I really need to work out is how to figure out if a label will spill into the margin so that I can automatically adjust the margin size.

Here's an example of some ouput of the current code. Sorry for the huge codedump!

use plotters::prelude::*;

const OUT_FILE_NAME: &'static str = "plotters-doc-data/layout2.png";

fn main() -> Result<(), Box<dyn std::error::Error>> {
    const W: u32 = 600;
    const H: u32 = 400;
    let root = BitMapBackend::new(OUT_FILE_NAME, (W, H)).into_drawing_area();
    root.fill(&full_palette::WHITE)?;

    let mut chart = ChartLayout::new(&root);
    chart
        .set_chart_title_text("Chart Title")?
        .set_top_label_text("A label at the top")?
        .set_chart_title_style(("serif", 60.).into_font().with_color(&RED))?
        .set_left_label_text("Left label")?
        .set_right_label_text("Right label")?
        .set_bottom_label_text("Bottom label")?
        .set_bottom_label_margin(10.)?
        .set_top_label_margin(10.)?
        .draw()?;

    let extent = chart.get_chart_area_extent()?;
    root.draw(&Rectangle::new(extent.into_array_of_tuples(), &BLUE))?;
    let extent = chart.get_chart_title_extent()?;
    root.draw(&Rectangle::new(extent.into_array_of_tuples(), &BLUE))?;

    Ok(())
}

siefkenj · 2021-05-28T20:53:45Z

This should be ready now. I currently I just pass a sub-drawing area where the chart area is to allow for graphing. I tried to figure out how to get tick locations so I could compute their size and automatically resize the chart, but it was just too hard and I couldn't find out the types.

For now we get an output that looks like

as a result of examples/layout.rs. I think maybe the automatic sizing of the tick labels should come in a separate PR, since this one s already getting pretty long.

Please let me know what you think!

examples/layout.rs

Cargo.toml

38 · 2021-06-01T18:02:29Z

src/chart/layout/nodes.rs

+/// Trait to constrain a type to a standard numeric type
+/// to prevent ambiguity in trait definitions.
+/// Idea from: https://stackoverflow.com/questions/39159226/conflicting-implementations-of-trait-in-rust
+pub trait Numeric {}


I am not sure if this is necessary. Also just curious if this is needed, can we just use https://docs.rs/num/0.4.0/num/trait.Num.html ?

That might work! I will try it.

It doesn't work. Numeric is needed to disambiguate between T: Numeric and S: Into<f32> as used in (S,S,S,S). Since a tuple could implement Info<f32>, the type system gets confused. Thats why I needed Numeric, which allows me to restrict to an explicit number type.

src/chart/layout/mod.rs

src/drawing/area.rs

38 · 2021-06-01T18:21:31Z

src/style/palette.rs

@@ -1,4 +1,11 @@
-use super::color::PaletteColor;
+use super::{color::PaletteColor, full_palette};


Note that full_palette is optional, so a conditional complication is required at this point

I thought that the colors being included in the prelude was what was configured but full_palette always existed?

38 · 2021-06-01T18:25:02Z

examples/layout.rs

+    chart
+        .set_chart_title_text("Chart Title")?
+        .set_chart_title_style(("serif", 60.).into_font().with_color(&RED))?
+        .set_left_label_text("Ratio of Sides")?


Also, this seems odd as we also have https://docs.rs/plotters/0.3.1/plotters/chart/struct.MeshStyle.html#method.x_desc which have the similar effect.

I guess this should be a broader discussion about how charts should be configured and what belongs to the chart vs. the coordinate system.

The way plotters seems to do it now is to attach labels to the coordinate system and then adjust whether the axes are printed to the left right/etc. Since ChartLayout doesn't handle coordinate systems right now, I just went an API that labels things based on position.

I think that maybe the best solution would be to have both. If you set via the coordinate system, it would automatically call the set_XXX_label_text. However if a person wanted to set the text without a coordinate system or to override it, they would be free to.

What do you think?

38 · 2021-06-01T18:45:26Z

Hi Jason,

Thanks for the PR, I am so excited to try this out. The general idea looks really interesting and I feel this actually makes plotters easier to use.

In general, I think the API workflow is a little bit odd. Currently, if my understand is correct, we need to build a ChartLayout and then pull out the chart_area from the layout then configure the chart context. But could we just build a chart context on top of ChartLayout?

I am pretty excited about what the current PR have, but I probably have some idea to tweak the workflow later. It looks good to me with the API if we can put it behind a feature switch.

Also I realized there are few warning introduced by the PR, we need to fix them.

Also, please don't mind I was not able to see this during the last weekend.

Thanks for this awesome PR!
Cheers!

src/chart/layout/mod.rs

siefkenj · 2021-06-02T02:19:50Z

Thanks for the review! I have a few additional changes related to automatic axis labeling. Do you want those changes pushed to this branch, or to a separate one?

In general, I think the API workflow is a little bit odd. Currently, if my understand is correct, we need to build a ChartLayout and then pull out the chart_area from the layout then configure the chart context. But could we just build a chart context on top of ChartLayout?

I tried to do this, but I got into a type/lifetime soup and couldn't figure out how to make it work, so I did what was easiest first. Maybe you can help me figure out how to get it working.

In my new work, I have automatic axis labeling work, with space being allocated for the axis labels. I couldn't figure out how to do this with the existing axis format, so I'll need some help figuring that out.

I am pretty excited about what the current PR have, but I probably have some idea to tweak the workflow later. It looks good to me with the API if we can put it behind a feature switch.

Of course! I fully expect you to tweak things :-)

a missing file

siefkenj · 2021-06-02T18:33:08Z

I have addressed most of your comments and included the additional code for auto-sizing the label areas (so that tick marks don't fall off the edge of the canvas). Unfortunately, to do that I had to create a new Axis type, because I could notfigure out how to extract the necessary information from Ranged.

Here's an animation of what it can do:

Notice how room is automatically allocated for the .0 on the 3.0. When the graph starts scrolling, the .0 doesn't fall off the edge and so the axis runs right up to the edge of the chart instead (it looks like a flaw in the animation, but it actually isn't!)

In order to achieve this, I needed to compute the extents of all the tick marks and tick labels. I then union them together and resize the tick_label_areas accordingly. If an extent spills to the right/left/top/bottom, I check that area to make sure there is already enough space allocated. If there isn't, I allocate more space.

This process is iterative because, for example, if a very wide left tick label is allocated, then this will affect the width of the x-axis and will potentially affect how many tick marks there are.

Right now, I only implemented everything in f32. You may want to replace the axis stuff with your Ranged objects, so I will refrain from making it general, for now.

siefkenj · 2021-06-02T18:37:59Z

For reference, here is what it looks like if a right label also exists. Since adequate space is always allocated, there's no bouncing!

38 · 2021-06-02T22:28:46Z

because I could notfigure out how to extract the necessary information from Ranged.

Just don't get the point. Could you please what is bouncing in the first animation ? Sorry for the ignorance

siefkenj · 2021-06-02T23:42:07Z

because I could notfigure out how to extract the necessary information from Ranged.

Just don't get the point. Could you please what is bouncing in the first animation ? Sorry for the ignorance

On the first GIF, watch the "Sine" label carefully as the number 4.0 scrolls by on the x axis. The Sine label is always aligned to the right of the chart. You can see it jump as the 4.0 passes by. That is the layout algorithm making room for the .0 on the right side.

38 · 2021-06-03T01:22:44Z

Ah, I see. But I'm not sure what information you need to avoid this. Probably I need to dig into your code a little bit.

siefkenj · 2021-06-03T01:40:10Z

Ah, I see. But I'm not sure what information you need to avoid this. Probably I need to dig into your code a little bit.

It's not a bug...it's how it is supposed to work.

The information I need relates to the separate issue of computing the extents of the axis labels. I couldn't figure out how to get the list of labels a Ranged was going to print, so I had to make a new struct, Axis, where I could figure out which tick labels were going to be printed so I could compute their sizes.

38 · 2021-06-03T17:39:21Z

src/chart/layout/mod.rs

+
+    /// Apply a cartesian grid to the chart area. Major/minor ticks are automatically
+    /// determined on a call to `draw`.
+    pub fn build_cartesian_2d(


This function should be generic, because we need to support all types of coords.

See https://github.com/38/plotters/blob/b82174aa1ac758ca6afb22a555d8732209fb2bb8/src/chart/builder.rs#L179

I don't know how to turn AsRangedCoord into Iterator<Tick>, which is what my code needs to estimate the size of the tick labels.

38 · 2021-06-03T17:41:48Z

src/chart/layout/mod.rs

+
+    /// Helper function for drawing ticks. This function will call
+    /// `draw_func(pixel_coords, tick, axis_side)` for every tick on every axis.
+    fn draw_ticks_helper<F>(


If my understand was correct, currently the tick is drawn by newly added code instead of the code in ChartContext?

That's correct. If I could turn an AsRangedCoord into an iterator that iterated through all the tick marks and labels, I could use the existing data structures to do it (this is what I couldn't figure out how to do).

I need to find the extents of each text item/tick mark to compute the correct amount of space.

Hmm, but I think we probably don't want having two different set of drawing code. If this is the case, I may need to think about unify those code into one. Also, I have the feel that this change seems a little bit big. So I think it's better to split it into parts.

Apologize for this, but I feel we should split the change into parts, since I think we are currently on a way leading to significant refactoring.

If this is the case, I am going to dig into commit 9938d7b, tweak a little bit and merge it. I will leave this PR open. After that let's figure out some better way to integrate rest part of the PR properly.

Please let me know what's your idea.

That sounds good. I can rebase from there after you've tweaked it.

38 · 2021-06-03T17:46:13Z

Ah, I see. But I'm not sure what information you need to avoid this. Probably I need to dig into your code a little bit.

It's not a bug...it's how it is supposed to work.

The information I need relates to the separate issue of computing the extents of the axis labels. I couldn't figure out how to get the list of labels a Ranged was going to print, so I had to make a new struct, Axis, where I could figure out which tick labels were going to be printed so I could compute their sizes.

See example https://github.com/38/plotters/blob/b82174aa1ac758ca6afb22a555d8732209fb2bb8/src/chart/builder.rs#L179

Use X:AsRangedCoord instead of Range<f32> for coord spec will allowing you get tick labels.

    x_spec.into().key_points(BoldPoints(n))

where n is the maximum number of tick marks.

I believe if you make ChartLayout::build_cartesian_2d generic, you are able to retrieve tick marks information.

siefkenj added 2 commits May 25, 2021 23:14

Initial work on flexbox layout

821a655

Merge remote-tracking branch 'upstream/master' into layout

a9a477c

Switch to concrete error type

5c0f3a5

Added ability to set labels and margins

b403db3

siefkenj added 2 commits May 28, 2021 16:38

Add accessor for chart_area drawing area

353c6a6

Fix incorrect calculation of drawing area pixels

9938d7b

Spelling fixes

79ec8f3

siefkenj changed the title ~~[WIP] Advanced Flexbox Layout~~ Advanced Flexbox Layout May 29, 2021

38 reviewed Jun 1, 2021

View reviewed changes

src/chart/layout/mod.rs Outdated Show resolved Hide resolved

siefkenj added 6 commits June 2, 2021 13:32

Added auto-sizing axes

45cb820

Added ticks module

6d81f7c

Merge remote-tracking branch 'upstream/master' into layout

6f23207

Put ChartLayout under a features flag

6514ed9

Call present at the end of example to make sure an error is thrown for

1b85ad0

a missing file

Autosizing of labels

a9e7037

Merge remote-tracking branch 'upstream/master' into layout

8cc45ea

38 reviewed Jun 3, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced Flexbox Layout #259

Advanced Flexbox Layout #259

siefkenj commented May 26, 2021

codecov bot commented May 26, 2021 •

edited

Loading

siefkenj commented May 26, 2021

siefkenj commented May 27, 2021 •

edited

Loading

siefkenj commented May 28, 2021 •

edited

Loading

38 Jun 1, 2021

siefkenj Jun 2, 2021

siefkenj Jun 2, 2021

38 Jun 1, 2021

siefkenj Jun 2, 2021

38 Jun 1, 2021

siefkenj Jun 2, 2021

38 commented Jun 1, 2021

siefkenj commented Jun 2, 2021

siefkenj commented Jun 2, 2021

siefkenj commented Jun 2, 2021

38 commented Jun 2, 2021

siefkenj commented Jun 2, 2021

38 commented Jun 3, 2021

siefkenj commented Jun 3, 2021

38 Jun 3, 2021

siefkenj Jun 3, 2021

38 Jun 3, 2021

siefkenj Jun 3, 2021

38 Jun 3, 2021

siefkenj Jun 3, 2021

38 commented Jun 3, 2021 •

edited

Loading

		@@ -1,4 +1,11 @@
		use super::color::PaletteColor;
		use super::{color::PaletteColor, full_palette};

Advanced Flexbox Layout #259

Are you sure you want to change the base?

Advanced Flexbox Layout #259

Conversation

siefkenj commented May 26, 2021

codecov bot commented May 26, 2021 • edited Loading

Codecov Report

siefkenj commented May 26, 2021

siefkenj commented May 27, 2021 • edited Loading

siefkenj commented May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

38 commented Jun 1, 2021

siefkenj commented Jun 2, 2021

siefkenj commented Jun 2, 2021

siefkenj commented Jun 2, 2021

38 commented Jun 2, 2021

siefkenj commented Jun 2, 2021

38 commented Jun 3, 2021

siefkenj commented Jun 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

38 commented Jun 3, 2021 • edited Loading

codecov bot commented May 26, 2021 •

edited

Loading

siefkenj commented May 27, 2021 •

edited

Loading

siefkenj commented May 28, 2021 •

edited

Loading

38 commented Jun 3, 2021 •

edited

Loading