[BUG] `DrawingArea::fill` doesn't fill the right and bottom edge in some backends

[shinmili]

Describe the bug
DrawingArea::fill doesn't fill the right and bottom edge in some backends and leaves transparent edges in a generated image. As far as I have checked, plotters-svg and plotters-canvas have this issue. BitMapBackend is fine about this.

For CanvasBackend, it's quick to see #165's attached image. It continues in the latest code too.

For SVGBackend, The repro code below prints this result. width and height in the rect element should be "100" here.

<svg width="100" height="100" viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">
<rect x="0" y="0" width="99" height="99" opacity="1" fill="#0000FF" stroke="none"/>
</svg>

Other backends with sub-pixel coordinate might have the same issue. Can be related to #128.

To Reproduce

use plotters::prelude::*;

fn main() -> Result<(), Box<dyn std::error::Error>> {
 let mut content = String::new();
 {
 let area = SVGBackend::with_string(&mut content, (100, 100)).into_drawing_area();
 area.fill(&BLUE)?;
 }
 println!("{content}");
 Ok(())
}

Version Information
plotters: current master (a72bfbc)
plotters-svg: current master (plotters-rs/plotters-svg@47289e8)
plotters-canvas: current master (plotters-rs/plotters-canvas@111fce5)

[shinmili]

plotters-cairo has the same issue. Adding the code below to the example in gtk-demo generated the red edge.

 // ↓ Added
 root.draw(&Rectangle::new(
 [(0, 0), (500, 500)],
 ShapeStyle {
 color: RGBAColor(255, 0, 0, 1.0),
 filled: true,
 stroke_width: 0,
 },
 ))
 .unwrap();
 // ↑ Added

 root.fill(&WHITE).unwrap();

[shinmili]

You need to update dependencies to see plotters-rs/plotters-cairo#20 works.

examples/gtk-demo/Cargo.toml

-plotters-cairo = "0.3.1"
-cairo-rs = "^0.9"
-gtk = "^0.9"
-gio = "^0.9"
+plotters-cairo = { git = "https://github.com/shinmili/plotters-cairo.git", branch = "fix-rect-width-height" }
+cairo-rs = "^0.15"
+gtk = "^0.15"
+gio = "^0.15"

examples/gtk-demo/src/main.rs

 let application = gtk::Application::new(
 Some("io.github.plotters-rs.plotters-gtk-test"),
 Default::default(),
- )
- .expect("Initialization failed...");
+ );
 
 application.connect_activate(|app| {
 build_ui(app);
 });
 
- application.run(&args().collect::<Vec<_>>());
+ application.run_with_args(&args().collect::<Vec<_>>());
 }

(When should this patch be merged? After publishing plotters-cairo to crates.io?)

[AaronErhardt]

Honestly, I don't know if we should fix the backends that don't work or whether we should rather fix plotters call to the backend. I think in this case the call to the backend is wrong. If the canvas is 100x100, the call would draw a rectangle with (0, 0) and (99, 99). And if the canvas was 0x0, the call would even draw a rectangle with (0, 0) and (-1, -1). That looks odd to me and is not how most backends work, as you pointed out in the issue.

plotters/src/drawing/area.rs

Lines 283 to 293 in a72bfbc

	/// Fill the entire drawing area with a color
	pub fn fill<ColorType: Color>(&self, color: &ColorType) -> Result<(), DrawingAreaError<DB>> {
	self.backend_ops(|backend| {
	backend.draw_rect(
	(self.rect.x0, self.rect.y0),
	(self.rect.x1 - 1, self.rect.y1 - 1),
	&color.to_backend_color(),
	true,
	)
	})
	}

The question is should we fix the backends that don't work or should we fix the backend that work? I'd say SVG, canvas and cairo were rightfully printing too few pixels, due to the -1. And doing a +1 on in each backend would only harm the rectangle drawing because that should be correct like it is.

[shinmili]

IMHO the real problem is that there is no serious vector graphics backend abstraction (as discussed in #128), and implementing DrawingBackend for SVG, canvas, or cairo backend has been done by, like, adjusting those vector graphics libraries to raster graphics model. I think the ideal solution would be reached by pushing #128 forward.

Looking around some codes and docs, I understood most parts of them assumes DrawingBackend as a framebuffer or raster graphics model. In that context, doing -1 to select the right-bottom end of a canvas is quite natural, just like doing arr.len() - 1 to index the last element of an array. That also explains why BitMapBackend just works fine; it doesn't need to fill the gaps between vector and raster. Not only this issue, but #165 too!

After a new trait for a vector graphics backend is introduced (or DrawingBackend is modified to support both raster and vector), vector graphics backends implement it, and plotters starts calling the new trait method, everyone will be finally happy. But it's not started yet and I guess it will take some time. I hope my patches would do until that day. They are a quick and effective fix that work with the current DrawingBackend, as long as it keeps its raster graphics model.

Or if we start #128 now, I'd like to help you, and would like to scrap those workaround patches now!

[AaronErhardt]

@shinmili I agree and will support you with improving the internal code for vector graphics backends. IMO vector graphics should be the "default" and raster graphics should have to adjust, not the other way around. I just don't have a lot time right now due to exams.

But there's one thing about the bitmap backend, I'm not sure whether it's actually correct:
In this example, we have a 6x6 canvas. First we plot a 5x5, then a 1x1 and then a 0x0 rectangle. However, the bitmap backend seems to increase their size by one pixel. You're right that in an array the last index is one less than the length, but the first index is also 0 (and 0 regarding images naturally means 0 size = invisible). IMO the SVG backend is correct here: 0x0 should not be visible and the 1x1 is just one pixel.

use plotters::prelude::*;

fn main() -> Result<(), Box<dyn std::error::Error>> {
 //let root = BitMapBackend::new("plots/0.png", (6, 6)).into_drawing_area();
 let root = SVGBackend::new("plots/0.svg", (6, 6)).into_drawing_area();

 root.draw(&Rectangle::new(
 [(0, 0), (5, 5)],
 ShapeStyle {
 color: RGBAColor(255, 0, 0, 1.0),
 filled: true,
 stroke_width: 0,
 },
 ))?;

 root.draw(&Rectangle::new(
 [(0, 0), (1, 1)],
 ShapeStyle {
 color: RGBAColor(0, 255, 0, 1.0),
 filled: true,
 stroke_width: 0,
 },
 ))?;

 root.draw(&Rectangle::new(
 [(0, 0), (0, 0)],
 ShapeStyle {
 color: RGBAColor(0, 0, 255, 1.0),
 filled: true,
 stroke_width: 0,
 },
 ))?;

 root.present()?;

 Ok(())
}

[shinmili]

@AaronErhardt OK, I had a confusion over how raster graphics should be, and now have changed my mind on what to fix: plotters and plotters-bitmap are the ones that should be fixed even in the current abstraction.

For some reason, I had been thinking raster graphics APIs should take coordinate ranges inclusively, and calling a maybe-out-of-bounds coordinate a "right-bottom corner" sounded really odd to me. In the context of your example, if btimap got fixed, (6, 6) would refer to an out-of-bounds pixel, but also be specified as a right-bottom corner coordinate to fill the whole 6x6 canvas. That's what sounded odd.

I stopped confusing after reviewing Win32 GDI API's Rectangle docs (last visited more than a decade ago):

Remarks

The rectangle that is drawn excludes the bottom and right edges.

So that's how most graphics libraries work, even in raster graphics. It's just fine to use (6, 6). I'll close the PRs and make ones for plotters and plotters-bitmap.

Thanks for your fruitful comments!

[facorread]

We really appreciate your contributions. Please bear in mind that plotters and plotters-bitmap are now hosted on the same GitHub repo.

Thanks!

[38]

Hi guys, thanks for the investigation. I think we should adjust bitmap backend to match SVG backend for consistency. @shinmili 's PR looks good to me, and I am also wondering if @facorread and @AaronErhardt have any idea about the PR.

And we need to clean up the plotters core code which assumes the rasterized backend - this is a legacy issue for a long time. Back to early days plotters was a program that renders PNG on a server. But since then plotters is becoming bigger and bigger - original rasterizing code was moved to plotters-backend as default rasterizer and plotters-bitmap was implemented as transparent optimization for framebuffer rendering.

Unfortunately, the different assumption have never been cleaned up and ending up those crappy bugs we are seeing today.

I strongly agree that we should have a new backend trait that really supports vector graphs. But this is a breaking change which means a major release of 0.4 - Once I have time I would definitely do that.

While at the same time, I am also want to really think about interactivity support as well. It's not really relevant to the thread, but I think it's a time for a road map to next major release which allows us to make breaking change and properly fix all the legacy issue.

Please let me know if you guys have any idea, and really appreciate for the awesome contribution!