Fully save all GraphicsContext variables for MacOS backend

[cimarronm]

Modifies the save/restore graphics context of the MacOS backend to fully save all GraphicsContext variables to a stack for the GraphicsContextMac class. Currently, the graphics context is saved to the stack in the underlying _macosx.so backend but the variables associated with the context from the GraphicsContextBase are not saved/restored so the corresponding GraphicsContext class is not in sync with the actual graphic context. This saves these variables to a stack so they are in sync.

This fixes the issue seen in #4024 (PR #4201 must also be applied to fully fix #4024). Below is the example from issue #4024 before and after.

Before

After

[tacaswell]

attn @mdehoon

[mdehoon]

This seems like overkill to me. At what point exactly does the graphics context get out of sync?

[mdehoon]

The graphics context gets out of sync due to the call to gc.get_rgb() in _draw_text_as_path in PathEffectRenderer.

[mdehoon]

This bug also appears with the gtkcairo backend.

[mdehoon]

I counted the usage of gc.get_rgb() in matplotlib overall. Other than in the function _draw_text_as_path in PathEffectRenderer, I only found it in backend-specific code. In general, calling gc.get_rgb() is not safe; depending on the backend it may or may not work.

[tacaswell]

@mdehoon Why does get_rgb() mess with the stack based context managers?

[mdehoon]

@tacaswell Because it simply returns self._rgb, self being GraphicsContextBase, and self._rgb is not saved on the stack.

[tacaswell]

Could the stack based context managers be modified to over ride to_rgb to return the correct value rather than the base value?

[mdehoon]

Yes of course, but that would be a huge mistake.

[tacaswell]

Why would that be a mistake?

[mdehoon]

Because it is an unnecessary complication of the code. As I wrote above, get_rgb is barely used anywhere in matplotlib, except in backend-specific code, which means that matplotlib can work perfectly fine without get_rgb.

[tacaswell]

It is not clear to me why #4532, which seems to be working around the fact that get_rgb does not work, is simpler than fixing backends.

How about over-riding get_rgb to raise NotImplemented for the backends where it is broken?

If get_rgb is broken how are things like https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backends/backend_macosx.py#L203 working?

[edited to fix horrible typo]

[mdehoon]

How about over-riding to_rgb to raise NotImplemented for the backends where it is broken?

I assume you mean get_rgb?

[tacaswell]

Yes, sorry just noticed that mistake.

[mdehoon]

How about over-riding get_rgb to raise NotImplemented for the backends where it is broken?

No no no, that would suggest that backends are supposed to support get_rgb. Just remove get_rgb from the matplotlib codebase altogether (except of course in backend-specific code if it is needed).

[tacaswell]

According to git-blame get_rgb has been there since 2004 (literally in
the first commit) so I am not enthusiastic about breaking that.

@mdboom @efiring

On Wed, Jun 17, 2015 at 10:10 PM mdehoon notifications@github.com wrote:

How about over-riding get_rgb to raise NotImplemented for the backends
where it is broken?

No no no, that would suggest that backends are supposed to support
get_rgb. Just remove get_rgb from the matplotlib codebase altogether.

—
Reply to this email directly or view it on GitHub
#4202 (comment)
.

[mdehoon]

Just move get_rgb to the backends that need it, and let other backends implement set_foreground in the way that is most appropriate for them. Since get_rgb is used in backend-specific code only, this is safe. Also user code should not be using get_rgb (though deprecating first before removing it is of course safer).

As a related point, we should also consider to let set_foreground accept rgb and rgba only, and not generic matplotlib color values. Most of matplotlib is already calling set_foreground with rgb or rgba, and I don't see a reason why set_foreground should allow for generic color values.

[tacaswell]

Converting all of the internal color storage RBGA tuples is a long term goal, but that will be a huge job.

After thinking about this for a bit, I greatly prefer this PR over #4532 as it address what I see as the real bug; that GraphicsContextMac does not correctly implement the GraphicsContextBase api.

If we want to have a discussion about changing the GraphicsContextBase api that is a discussion for when we do a v3.0. Change that API on a point release is a non-starter for me and we are committed to 2.0 being style changes only.

so 👍 on merging this from me, but I would like some more eyes on this.

[efiring]

I agree with @tacaswell that this PR makes sense given the long-established GraphicsContextBase API; I don't see any downside to it other than a little bit of overhead, regardless of any later changes to low-level color handling and general backend APIs.

[mdehoon]

Sorry guys, but this PR is unacceptable to me.

[efiring]

@mdehoon We have to come to a better understanding of how to proceed, recognizing that it might take several steps. I don't yet understand your strong objection to the present PR as a first step. In particular, you have not answered @tacaswell's question above: "If get_rgb is broken how are things like https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backends/backend_macosx.py#L203 working?" Doesn't the present PR fix a problem that could otherwise occur with that line? In other words, isn't the macosx backend relying on the behavior that this PR will produce, which is consistent with the long-standing API?

[mdehoon]

@efiring Sure. All my scientific work is done with matplotlib (with the MacOSX backend on Mac and with other backends on Linux), so I really hope to arrive at a solution that is best for matplotlib in the long run.
To answer @tacaswell 's question: If set_foreground is called first and set_alpha soon thereafter, then get_rgb will return the correct value. There are situations where this will break, for example if somebody does this:

gc1.set_foreground('red')
gc2 = renderer.new_gc()
gc2.set_foreground('blue')
gc1.set_alpha(0.5) # gc1 will think it's blue

I am not aware of any cases where this kind of pattern appears in matplotlib, so this doesn't manifest itself as a bug. So it works in practice, but I agree that fundamentally it is a bug, which I would like to see fixed.

Doesn't the present PR fix a problem that could otherwise occur with that line?

Yes. However, matplotlib suffers from API bloat and is in places unnecessarily complex. Accepting this PR would further add to the complexity of the code. And the more complexity we add, the harder it will get to disentangle it later. Let's reduce some of the complexity of matplotlib first, making it more reliable, less prone to bugs, faster, and more maintainable.

[efiring]

@mdehoon Thank you for the explanation. I think everyone agrees with you that "matplotlib suffers from API bloat and is in places unnecessarily complex", so the question is just about how to improve the situation.
Looking at #4532, I see an increase in complexity and a reduction in consistency: in a place where the PathEffectRenderer now encapsulates a nice chunk of functionality, that encapsulation is replaced by lower-level code, for no obvious reason. It leaves in place the gc API inconsistency that is fixed by the present PR. Neither PR is adding to the API bloat. By improving consistency, the present PR is, if anything, making it easier to simplify the API in the future.
In summary, I think we agree on the goals, but I reach a conclusion opposite to yours: I find the present PR to be a good step, and #4532 to be unacceptable. What's wrong with my rationale, as stated above?

[mdehoon]

@efiring I am OK with abandoning PR #4532 .

I can explain in more detail why the current PR is unacceptable, but I think it is easier to see after we deal with gc.set_foreground. I don't insist that this should happen at the next point release, but if the long-term goal is to convert to rgba tuples for internal color storage, then we will have to deal with it at some point.

[tacaswell]

@mdehoon We would like to cut a 1.5 in the next month or two and I would rather not ship a broken backend if it can be avoided. Is there a more minimal patch (like #4542) that we can use as a stop-gap?

[mdehoon]

@tacaswell No there isn't. But I wouldn't worry too much about it. This is not a major bug and few matplotlib users will run into it.

[mdehoon]

To get started, how exactly is graphics context . set_foreground defined?
The docstring says "Set the foreground color." Does that mean the stroke color, the fill color, the text color, or all of these?
As a related question, how is set_alpha defined in relation to set_foreground? In particular, if I do
gc.set_alpha(0.5)
gc.set_foreground((1.0, 0.0, 0.0, 1.0), isRGBA=True)
then should the alpha be 1.0 or 0.5? The code in backend_bases.py suggests it should be 0.5. But that seems utterly wrong.

[efiring]

@mdehoon, mpl evolves gradually, with a long history of trying to minimize breakage of user code. The handling of transparency has been a difficult part of that transition, as the importance and usage of transparency have increased over mpl's lifetime. I think JDH started mpl largely based on the gtk drawing model as it was a dozen years ago. The backend code has to do its best to match the mid-level Artists to the actual drawing models of the various backends, including ps, which doesn't support alpha at all. A number of years ago we were running into big problems with alpha, and the sort of ambiguity you refer to. The partial solution was to keep track, in the GC, of whether the overall alpha should override the A in an RGBA value. That's what _forced_alpha is for. Look in backend_bases.py to see how it works. Similarly, look there to see how gc.set_foreground((1.0, 0.0, 0.0, 1.0), isRGBA=True) is handled. All of this was the best we could come up with as a compromise between the existing codebase and user code, and the need to handle RGB, RBGA, and various ways of specifying alpha outside of RGBA. And provided the backends conform to the backend_bases model, it works reasonably well. Mpl has a lot of users, and there hasn't been a flood of complaints about our handling of transparency--though, no doubt, it could be clarified and improved. A well thought out MEP for this would be most welcome.

Regarding your first question: it turns out that "foreground" is just the color for whatever is being drawn with the gc. Yes, it turns out that it might be better to have separate color attributes for fill and for stroke, but that's not the way this thing evolved. Until a major overhaul is done, we have to get along as well as possible with what we have.

So the situation with respect to the present PR is that a bug has turned up, specific to the macosx backend, and specifically because that backend does not adhere to the long-established (if imperfect) model in backend_bases. The present PR adds two small methods, 9 substantive LOC, that bring it into compliance--that make it consistent with the model, in that the python-side gc is in sync with the extension code gc. I thank @cimarronm for identifying the underlying problem and providing a concise explanation and solution for it. I haven't seen any comment indicating that there is anything wrong whatsoever with the diagnosis and the cure. As @tacaswell has noted there is one even simpler but less thorough alternative, now applied to the cairo backend. Presumably it would also handle the present macosx backend bug.

[mdehoon]

Guys, let me try and explain more detail why I don’t want to accept this patch:

1. The bug occurs in the call to gc.get_rgb() in _draw_text_as_path, because
2. the MacOSX backend does not guarantee that gc.get_rgb() returns the correct rgb value.
3. The get_rgb() has been part of the matplotlib API for more than ten years.
4. But in the current codebase, assuming the MacOSX backend is being used, I am finding get_rgb in _draw_text_in_path, gc.set_alpha, and gc.set_foreground only. The latter two cases are trivial; so this leaves _draw_text_in_path as the only piece of code that needs get_rgb.
5. This means that in more than ten years, matplotlib has not found a good use case for get_rgb. Some specific backends may use it as they see fit, but there doesn’t seem to be a general need for it.

The options now are to either (A) update the MacOSX backend to support get_rgb and to comply with the long-standing API, or (B) remove get_rgb from the API because there is no need for it.

The proposed patch ensures that get_rgb returns the correct value. However, note that the MacOSX backend does not use get_rgb for drawing, but relies on gc.set_foreground only. So this does not provide a general solution; there will be cases where the tkagg and the MacOSX backend will behave differently even after accepting this patch.

While appreciating @cimarronm ’s efforts, this patch implements an API that shouldn’t exist in the first place. Accepting this patch now will mean having to remove it later.

For this particular bug, the solution is simple: Don't use get_rgb. No other part of matplotlib needs it, so I presume that we can implement PathEffectRenderer without using get_rgb. My patch was perhaps a bit radical; there are alternative solutions to solve this bug that are less invasive.

[efiring]

@mdehoon, thank you for the very clear and explicit explanation.
The usage of get_rgb in PathEffectRenderer is identical to its use in RendererBase: it is in _draw_text_as_path, which is called directly by draw_text and draw_tex. These public API methods would need to have an additional rgbFace argument if that information cannot be retrieved from the existing gc argument. That strikes me as a highly invasive API break; I don't see any easy way to get around it. In other words, the present use case for get_rgb is highly localized, but that doesn't appear to make it easily removable. Nor do I see why it is so desirable to essentially make the GC API write-only by removing, or considering as optional, the property read methods.
Returning to your correct identification of options A and B: A is simple and immediate. B is really an unknown--we have no candidate implementation, so for now it is a "do nothing" option. As you note, if A is adopted, it will be undone if B is adopted at a later date, when there is an implementation. That seems like a weak objection to adopting A now; it's only a few LOC, all in one place.

[efiring]

While testing this with the #4609 problem, I encountered an exception. I was not able to reproduce it on master, so it looks like it is triggered by this PR, after closing the test figure window based on #4609 and manipulating another figure. Unfortunately I didn't capture the traceback and I don't have time now to regenerate it. I think it was related to a reference to a non-existent GC from the original plot, though.

[tacaswell]

Punted as this is not the right solution.

@mdehoon For this use case I think the get_rgb is called with in a draw context so overriding get_rgb on the osx gc to defer to the underlying c-layer object would work.

It might also be worth deleting the default attributes on the gc.

[mdboom]

Thanks for working this out @cimarronm. Given all the other issues with the macosx backend, I think we're leaning toward just using the Agg renderer which would fix this and a host of other things.

Obsoleted by #6178, which uses Agg for rendering the OSX backend and thus works around the bug in a different way. (Fixed by #6178).