Advanced Flexbox Layout #259
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
I think I figured it out...I switch to concrete error types instead of |
|
@38 The bulk of the API should be implemented now. Please let me know what you think. One question I had is about the So far I have implemented labels and extents, but not yet the passdown to the sub-region ( 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(())
} |
|
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 Please let me know what you think! |
| /// 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 {} |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
That might work! I will try it.
There was a problem hiding this comment.
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.
| @@ -1,4 +1,11 @@ | |||
| use super::color::PaletteColor; | |||
| use super::{color::PaletteColor, full_palette}; | |||
There was a problem hiding this comment.
Note that full_palette is optional, so a conditional complication is required at this point
There was a problem hiding this comment.
I thought that the colors being included in the prelude was what was configured but full_palette always existed?
| 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")? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
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 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! |
|
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?
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.
Of course! I fully expect you to tweak things :-) |
|
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 Here's an animation of what it can do:
Notice how room is automatically allocated for the In order to achieve this, I needed to compute the 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 |
|
For reference, here is what it looks like if a right label also exists. Since adequate space is always allocated, there's no bouncing!
|
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 |
|
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 |
|
|
||
| /// Apply a cartesian grid to the chart area. Major/minor ticks are automatically | ||
| /// determined on a call to `draw`. | ||
| pub fn build_cartesian_2d( |
There was a problem hiding this comment.
This function should be generic, because we need to support all types of coords.
There was a problem hiding this comment.
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.
|
|
||
| /// 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>( |
There was a problem hiding this comment.
If my understand was correct, currently the tick is drawn by newly added code instead of the code in ChartContext?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That sounds good. I can rebase from there after you've tweaked it.
See example https://github.com/38/plotters/blob/b82174aa1ac758ca6afb22a555d8732209fb2bb8/src/chart/builder.rs#L179 Use where I believe if you make |
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 thestretchlibrary. I am now working onChartLayoutwhich should be similar toChartBuilderbut with additional flexibility. However, I could use some pointers...Something with how I am doing error handling in
ChartLayoutisn't right. Inexamples/layout.rs, if I use?to unwrap the errors, I get a compile error:which I assume is because the error is somehow related to the
DrawingAreato 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?