Implement fill for circles and rectangles #2
Conversation
src/lib.rs
Outdated
| color: convert_color(style.color()), | ||
| }); | ||
| if fill { | ||
| for radius in 1..=radius { |
There was a problem hiding this comment.
I am concerned that there may be empty pixels not covered by any circles due to rounding issues. What about we draw line by line with pyth them here?
for dy in -radius..=radius {
let half_width = ((radius.pow(2) - dy.pow(2)) as f64).sqrt();
self.draw_line(
((coord.0 - half_width) as i32, coord.1 + dy),
((coord.0 + half_width) as i32, coord.1 + dy),
style,
);
}There was a problem hiding this comment.
Good catch. Didn't show up in the small circles I tested in my application, but very much does at larger scales. Your solution works well.
I've added a test rig and will add as a commit in my PR, and this demonstrates that unfilled circles don't work. Do you see them working?
There was a problem hiding this comment.
yeah, I am planning to send a PR to ratatui to fix their implementation.
src/lib.rs
Outdated
| let (mut start, mut stop) = | ||
| if coord1.0 < coord2.0 { (coord1, coord2) } else { (coord2, coord1) }; |
There was a problem hiding this comment.
Does this mean we don't care about the coord1.0 < coord2.0 && coord1.1 > coord2.1? Why aren't we ordering them independently?
| let (mut start, mut stop) = | |
| if coord1.0 < coord2.0 { (coord1, coord2) } else { (coord2, coord1) }; | |
| let (mut start, mut stop) = ( | |
| (coord1.0.min(coord2.0), coord1.1.min(coord2.1)), | |
| (coord1.0.max(coord2.0), coord1.1.max(coord2.1)), | |
| ); |
There was a problem hiding this comment.
Independently ordering coord1 = (2,3); coord2=(1,4); would transformed them in to completely unrelated coordinates: start = (1,3); stop = (2,4);
There was a problem hiding this comment.
Yes, quite. I woke early today with my subconscious thoroughly embarrassed that yesterday wasn't a day for me to be doing even the simplest Cartesian geometry! 😳
There was a problem hiding this comment.
Hmmn. This appears to work on a visual inspection of the tests I included, but not with my application.
Or maybe this is down to moving from Ratatui 0.25 → 0.26 …
|
Consider contributing the fill implementations to ratatui instead of this repo? After all, these are quite general-purpose requirements that might involve some more internals. |
|
I added the corresponding fill fields in ratatui/ratatui#919 and ratatui/ratatui#920 |
The examples file will not compile without this.
After rebasing against recent master changes, running `cargo test`
elicits the following error:
❯ cargo test -- --test-threads=1 --nocapture
Compiling libm v0.2.8
Compiling num-traits v0.2.17
Compiling getrandom v0.2.12
Compiling strum_macros v0.26.1
Compiling castaway v0.2.2
Compiling static_assertions v1.1.0
Compiling ppv-lite86 v0.2.17
Compiling itoa v1.0.10
Compiling ryu v1.0.16
Compiling rand_core v0.6.4
Compiling compact_str v0.7.1
Compiling rand_xorshift v0.3.0
Compiling rand_chacha v0.3.1
Compiling rand v0.8.5
Compiling image v0.24.8
Compiling chrono v0.4.33
Compiling rand_distr v0.4.3
Compiling strum v0.26.1
Compiling ratatui v0.26.0
Compiling flexi_logger v0.27.4
Compiling plotters-bitmap v0.3.3
Compiling plotters v0.3.5
Compiling plotters-ratatui-backend v0.1.3
(/home/ebardie/src/bytecounter/plotters-ratatui-backend)
error[E0601]: `main` function not found in crate `boilerplate`
--> plotters-ratatui-backend/examples/boilerplate.rs:54:2
|
54 | }
| ^ consider adding a `main` function to
`plotters-ratatui-backend/examples/boilerplate.rs`
For more information about this error, try `rustc --explain E0601`.
error: could not compile `plotters-ratatui-backend` (example
"boilerplate") due to 1 previous error
warning: build failed, waiting for other jobs to finish...
This commit adds a dummy main to `examples/boilerplate.rs`, which keeps
`cargo` happy.
| @@ -12,7 +12,7 @@ description = "Ratatui widget to draw a plotters chart" | |||
| plotters-backend = "0.3.5" | |||
| ratatui = "0.26.0" | |||
| thiserror = "1.0.50" | |||
| plotters = {version = "0.3.5", optional = true, default-features = false} | |||
| plotters = {version = "0.3.5", optional = true, default-features = true} | |||
|
|
||
| When running tests please note the following constraints. | ||
|
|
||
| The tests *require* human interaction. An image is displayed in the upper section of the terminal, with a yes/no question in the lower section. Inspect the image, and respond to the question with 'y' or 'n' as appropriate. |
There was a problem hiding this comment.
Umm, these are called examples not tests. Tests should be runnable in CI.
There was a problem hiding this comment.
You may want to look at how ratatui tests its own widgets,
There was a problem hiding this comment.
I humbly disagree - these are tests not examples. I didn't like them needing a human to perform the pass/fail assessment, but I found them more helpful than having no tests :)
Thank you for the pointer to Ratatui's test capabilities. I'll rework and resubmit at some point.
Currently the fill flag is just ignored. This PR is for a simple fill implementation for circle and rectangle.