This looks far better!

I don't see why anyone would care if their raster in a vector graphic was 300.5 dpi instead of 300, but perhaps we should make sure there are no practical reasons that is a problem.@jkseppan is the PDF backend expert and may have some thoughts.

So do those methods all really need to be re-implimented? Before you do all that can we check that you are doing this the easiest way?

I took a quick look at the code this morning, and it does seemsomething will have to bee added becausedraw_image loses track of how exactly how wide the image is meant to be once it figures out how many "pixels" wide it will be. That makes perfect sense for actual raster outputs, but is not applicable here.

Thanks for helping out Jody!

So do those methods all really need to be re-implemented? Before you do all that can we check that you are doing this the easiest way?

The methods don't need to be reimplemented, we just need to write small little wrappers that compute the bbox in each case, but would also be happy to hear if there is a cleaner way!

It's unfortunate, because at the level of the individualArtist.draw calls, the artist always has aget_clipbox that could be read directly to grab the "real" bbox. But the first call that touches the renderer is arenderer.draw_* call, and none of the parameters of these methods have information about the size of the object (so I have to recalculate it in my wrappers.

For example the colorbar is aQuadMesh, so even though we know its absolute size inside ofQuadMesh.draw (via e.g.QuadMesh.get_clip_box), onceQuadMesh.draw callsrenderer.draw_quad_mesh, (which I wrap), I have to recompute theBbox by hand, which is very wasteful.

Do you really need to pass in the true bbox explicitly? Can the vector backends not infer it by applyingtransform to(x, y, im.width, im.height)? Possibly the caller may need to adjusttransform a bit, though. But then no need for signature changes.

Do you really need to pass in the true bbox explicitly? Can the vector backends not infer it by applyingtransform to(x, y, im.width, im.height)? Possibly the caller may need to adjusttransform a bit, though. But then no need for signature changes.

As far as I can tell, the information about what the original size of the figure is has been lost by the time that you reach the vector backend. We could in principle hijack thetransform kwarg to get that information in there....it's not hard to construct the correct translation+scaling (as in the current PR's version ofbackend_svg.draw_image).

I'm happy with whatever everyone agrees is the best/minimal change to the interface.

However, the bigger issue is that it looks like I have to make an API change of some sort anyway in order to have the actual extents available to pass todraw_image in the first place.

There are two-ish places wheredraw_image is called (inimage.py:_ImageBase.draw andbackend_mixed.py:MixedModeRenderer.stop_rasterizing). This PR already correctly deals with the latter case, but the former case actually not quite.

What I currently do (use the Bbox of theparent passed to_draw_list_compositing_images) works forFigureImage andAxesImage objects because in those casesparent is theFigure/Axes). However, forBboxImage's, that is not necessarily the correct bounding box.

Solutions I can think of:

1. Bumpget_window_extent to the superclass_ImageBase, so that we can call it in_ImageBase.draw to get the correct pre-rounding bbox to pass to the vectorized renderers

Happy to hear if people approve and/or have better ideas?

That said, maybe in terms of scope, we could leaveimage.py as-is in this PR, and only modify thedraw_image call inMixedModeRenderer.stop_rasterizing. Then, any_BaseImage subclass will still be drawn not aligned to its edge, but at least we'll have fixed the bug for every other rasterized object thatMixedModeRenderer deals with (color bars in particular)?

May be easier to discuss this over a call (at least right now I have trouble keeping track of all moving parts); let's try to discuss this on Monday or possibly separately?

Having a look at it again briefly (sorry for doing this in an on-and-off manner), I think you can indeed just smuggle the relevant information in thetransform kwarg (and hence have no external API change) by doing the relevant work in make_image/_make_image?

Not at all, thanks for looking into it! You're right, I'll push up a version with no external API change before the meeting tomorrow :)

we could leaveimage.py as-is in this PR, and only modify thedraw_image call inMixedModeRenderer.stop_rasterizing

Decided that in order to keep this PR sane, I have to restrict it to only fix half of the "bug".

The new version I just pushed fixes the colorbar bug, but leaves the imshow bug as-is. Separate PR for imshow bug incoming.

My new fix seems to work great,unless someone requests a rasterized Path during a call to a MixedModeRenderer and the linewidth is larger than a few pixels (no idea why you would do such a thing, since the whole idea behind mixed mode renderers is to be able to have paths vectorized).

This corner case has a regression (ironically, intests/test_image.py:rasterize_10dpi_pdf).

The lines in the middle and on the right are supposed to look the same:

The issue is that my new code uses the following trick:

1. keep track of the bbox of each rasterized area by wrapping calls todraw_marker,draw_path,draw_quad_mesh, etc. inMixedModeRenderer.
2. at the end of each rasterization, scale the rendered pixels to the final "theoretical" bbox we've calculated.

That works great, except for in order to calculate the correctbbox for a stroked path, we need to wait for the code in#17198. Right now, although we merged some fixes to path extents calculations, we still assume a "zero" thickness path when computing its extents.

#17198 is abig PR. But until it gets merged, we need to accept this regression (the users that would trigger such a thing, I'm not very sympathetic to, personally, whereas I do think we should be able to render colorbars without them being super ugly) or come up with a whole new strategy, that doesn't involve pre-computing where each rasterization will be placed in the final vector image.

The otherbaseline_images changes are "more" correct than they used to be.

@brunobeltran where is this PR at?

ping@brunobeltran This will fix alot of long-standing bugs. If you have abandoned it, please let us know and someone will pick it up...

I'll ping again, and marking as stale...

I've added to the 3.6 milestone and to the project board as I think this is quite high priority. Its a little ridiculous that our vector graphic rasters snap to virtual pixels.

ping@brunobeltran - I'll take this over if you can't get back to this as its pretty high priority. We get bug reports every couple of months about this...

pro: I think I got the rebase to work!
con: there are a bunch of test failures I do not understand locally

ok understand one of the errors:

• this PR does not wrapdraw_markers yet

Unfortunately I think this is going to have to get punted a version again.

This version passes all tests, and adds one more test to ps backend. Obviously some image changes.

It does not wrap markers so that is still a todo, but it doesn't make the situation worse.

@anntzer suggest wrapping this in thetransform object. I think thats true, but seems more confusing than handling this directly.

@anntzer suggest wrapping this in the transform object. I think thats true, but seems more confusing than handling this directly.

I only looked at this briefly again. Looks like since my last comment (and probably since#17182 (comment)) the API changes are now limited to the addition of the true_size kwarg which effectively(?) serves as a private communication channel between MixedModeRenderer and the builtin vector backends, which is not so much of an issue (at least on mplcairo's side I just completely skip trying to work with MixedModeRenderer...).

Still it's not entirely clear to me what the semantics ofdraw_image(gc, x, y, im, transform, true_size) now really means. Previouslydraw_image(gc, x, y, im, transform) meant "draw the image at position x, y (in pixels); the image pixels (0..m, 0..n) should be transformed by transform first and that also indicates the size of the rendered image". How does that now interact with true_size? Perhaps that's simple enough, but should be documented.

Minor review points: true_size should probably be true_shape (for consistency with ndarray.shape/ndarray.size), and option_true_bbox_image is no longer needed.

Still it's not entirely clear to me what the semantics ofdraw_image(gc, x, y, im, transform, true_size) now really means.

I think that is unchanged. The new codepaths are only iftransform is None.

I think that is unchanged. The new codepaths are only if transform is None.

Fair enough, but this should then be documented.