Some design decisions that I didn't know how to make:

1. I implemented aget_centered_bbox function. Do I implementget_extents, orget_bbox, or something else instead? There doesn't seem to be an agreed-upon convention in the codebase for how to extract the bbox information from an object (lines haveget_window_extents, paths haveget_extents, etc.)
2. the information to calculate thePathEndAngles for each glyph cantheoretically be extracted from the path itself. I opted to manually tabulate them instead since the number of glyphs is small and they are geometrically straightforward. A more generalized approach that identifies the points on the path that "create" the bbox, then extracts their PathEndAngles from the path itself would be more general and theoretically resilient to future/custom glyph types, but would be quite a bit more work.
3. more in general, I'm not familiar enough with the mpl codebase as a whole to know if there's already a convention for aggragating data like I do using BoxSides(PathEndAngles(...))
4. On that note, as long as we don't do something like suggested above in (2), we will now be storing more than two per-glyph-type data things (and if e.g.Sizes of different markers are not perceptually uniform #15703 gets implemented we'll have a couple of more pieces of per-glyph-type information, the area and visual scaling constants, saved as well). This might argue for a simple refactor of the currentmarkers dictionary into a list ofGlyphInfo objects, containing (for each glyph) the
   a) list of valid short names (e.g. ['', ' ', 'None', None])
   b) long name (e.g. 'star')
   c) PathEndAngles (or whatever they end up being called)
   d) area of glyph, to scale with
   e) (in the future) effective visual size scaling constant

I'm happy to do that simple refactor here, since I'm adding the information, but wanted to make sure there was a shared feeling that it is necessary in the first place, and that I wouldn't be wasting too much time doing it in a style that would immediately be rejected.

Just code that makes a plot that's composed of one marker with known size and edge width and then check that get_tightbbox returns the right values?

I don‘t know if it‘s necessary/worth to distinguish different markers.

Now that it's more obvious that this gets messy on a per-glyph basis, should we include one test per glyph or just test a couple of the nasty ones? (say, 'p', '*', 'o', 'x', to get examples of glyphs with miter joins, round joins, filled and not)?

Weird. The tests I just added pass in a Jupyter notebook but not with the Agg backend or on Travis?

Basically, the code

_draw_marker_outlined('*', markeredgewidth=20)

produces a 480x480px image with the box in the wrong place.

The calling the identical function in Jupyter...

test_marker._draw_marker_outlined('*', markeredgewidth=20)plt.savefig('marker_bbox_star.png')

produces a correct figure, but 345x345px.

I don't think this is a problem in my own code, since the box is incorrect even for the trivial case ofmarkeredgewidth=0, and doing the following in Jupyter produces a 480x480px image that is also correct

test_marker._draw_marker_outlined('*', markeredgewidth=20)plt.savefig('marker_bbox_star.png', dpi=100)

@timhoffm Ready for re-review and need help.

1. My new tests are passing in Jupyter, but not in the console (see above).
2. I ended up adding a new public API elementMarkerStyle.get_centered_bbox, but don't know where or how to document it appropriately besides its docstring.

I think this is a dpi issue. Some calculation does not Takt them into account. The jupyter backend (%matplotlib inline) sets 72dpi by default, everything else uses 100dpi. What happens if you doplt.rcParams["figure.dpi"]?

Sorry for the formatting, I‘m on mobile.

I‘m quite busy right now. Please be patient with a full review. That may take a couple of days.

Thanks for the quick reply Tim! Looks like it is a DPI thing, I'll follow that lead.

Of course I assume you're very busy, and I definitely wasn't meaning to rush you. I just wanted to ping so you knew it was safe to start looking without wasting even more of your time.

Thanks in advance for your help!

You removed the linems = (self._markersize / 72.0 * self.figure.dpi) * 0.5 which accounts for dpi. So the box will be wrong for every dpi setting, except72, where1/72*dpi==1.

Haha, didn't see the "member" when I replied to you earlier@ImportanceOfBeingErnest. Hopefully I didn't come off as presumptuous!

Mostly, thanks for the good eye! I had forgotten that the markers stuff is in points. Fixed the code to make that consistent. Tests should pass now. Had to push up new "baseline_images", since they were generated at different DPI than before for whatever reason, but now look as expected.

To have image comparisson look like the default style one can use

@image_comparison(..., style='mpl20')

...just in case that is still relevant. One could also try to use only a single image (image comparison is rather expensive, so if it can be reduced to a minimum, that would sure be nice)

In general I haven't quite understood how wrong adding half the edge width would be and if that really warrants adding this huge code block. In particular, I would have though adding half the edgewidth would ratheroverestimate the bbox - which would be totally acceptable in my eyes.
Maybe an example case either here, or in the original issue#16606 would be helpful in that respect.

I agree with@ImportanceOfBeingErnest. This is for the rare case that a marker over-spills its axes, and most folks don't have markeredgewsidths that are much larger than a point or two. This is really cool that you sorted through all the geometry, but is it overkill compared to the actual problem?

If that's the problem, I'm happy to write a slighly more general solution that computes these properties from the path of the marker. It is possible, just a little difficult in the general case as computing the extents of stroked bezier paths is famously complicated. However, I do know how to do it correctly, and since I use a lot of custom markers, it would greatly benefit me since my fix would then apply in that case as well.

But I don't think we need to cover the general case here, just polygons and circles. I guess polygons would be relatively simpler; for circles, somehow luckily, falling back to 0.5*mew would work :-)

If I do this, will it get mainlined?

Well that's the problem as always: any code that goes in needs to be maintained and is extremely annoying to get rid of, and maintainer time is finite. That's why we're not necessarily so keen on adding a few hundreds of line of code just to handle a relatively edge case :-) (even though it's quite impressive work, as mentioned just above :-))

Conveniently, the currently hardcoded figure properties mean we would have amazing test coverage of this more general solution with no extra work I suppose....

Not a huge fan of playing the coverage numbers game :-)

@anntzer , I got a little swamped with paper deadlines, but started working on code for general solution today (written, just needs testing).

However, I ran into a bit of a design issue. Right now bezier.py imports path.py (but only in a couple of functions that feel to me like they should be in path.py anyway...). Obviously a general routine that calculates bbox for a stroked path belongs as a method of Path. But I need helper routines from bezier.py so....

Should I

1. put my code in bezier.py/markers.py (as in current commit, less elegant, but no API breaking)]
2. fix the underlying issue, move appropriate code from bezier.py into path.py, do it "right" (internal API break, but that code is really only used in one place internally...)

in order to minimize resistance to this already pretty contentious PR?

I would be fine with moving stuff from bezier.py to path.py if that's necessary.
You may consider breaking stuff into multiple consecutive PRs to prevent review from going out of control... ;)

Before there‘s going more work into this: Are we positive, that we want to take on the exact solution with the added amount of code vs. an approximate solution? It would be a shame to detail it all out and later decide that the maintenance burden would be too high.

I haven‘t looked into this in detail, but the amount of code needed to special-case the exact solution scares me a little.

Ping@jklymak@anntzer@tacaswell@ImportanceOfBeingErnest I think we should have a champion for this. If nobody is stepping up for it, I fear the PR will have a hard time of getting merged.

I think this will depend on the amount of code involved... (I can't guarantee a quick review, but will try to keep a look on this.)
I just realized there may be an easier solution: Ithink the "true" size of a marker likely always grows asa*markersize+b*markeredgewidth+c? in which case we could just get away with estimatinga,b andc by rasterizing the marker at a few ms/mew values (the results can be cached per-marker), instead of doing the complicated geometry calculations.

Yeah something like@anntzer's proposal was actually my plan after you guys asked about performance, just didn't want to dump a bunch of work into this if it wasn't going to get accepted, as the current implementation works just fine for my purposes.

The current recommendation is:

1. Fix Path/Bezier to depend on each other in a way that makes sense, clean up redundant code (separate PR).
2. Additer_corners method to compute geometrical properties directly from the Path.
3. Cache constants on MarkerStyle creation (a/b/c, as above) that describe how marker Bbox scales withmarkersize/markeredgewidth.
4. AddMarkerStyle.get_extents(renderer) to get exact bbox without slowdown compared to current method.

Superceded by#17119, a cleaner implementation using some of my recent additions toPath.