Worked out how to parametrize the test.

Let me know if you'd like to see the test in a separate PR, to make the result easier to compare.

Edit: Done in #12273 - will rebase this after that.

Would it be possible to stick the test images on different axes on the same figure (something like subplots(2, 2)? This will save a few images, and also make a nice side by side comparison of the different viewing angles.

I avoided that because it leads to a 100% diff in the target image, and also removes the ability to use pytest.parametrize.

However, I'm not super committed to that decision - if you think that'd be best, can you comment to that effect on #12273?

@dstansby: How does #12292 look?

Updated. Something odd seems to be happening though - the light source is above the plot, yet it renders as if it were below. - resolved - _generate_normals was wrong

Might also be worth considering changing the default lighting angle so that it doesn't hit cuboids at exactly 45 degrees causing adjacent faces to be the same color.

I would have thought that the reason for the two bright faces is that, independent of the viewing angle, you have one bright face facing the viewer. Is this undesireable?
I think it would be good to see the same case visually with a before/after - comparisson here.

@ImportanceOfBeingErnest: you have that before/after comparision in the rich diff of the result image

An argument for why this should be fixed - for graphs containing both a surface and a bar plot, this will make the shading consistent.

So for surface plots the lightsource rotates together with the coordinate frame as well. I never noticed that, but now that you mention it, it's clear. (Somehow undesired in my eyes, I would much rather have the lightsource constant in the room and rotate the plot alone, instead of rotating the whole room with its lights, but that's not for debate here I guess.)

Would there be any way to make it consistent for the zero-extent bar as well?

Ugh, I didn't see you'd replied here.

Somehow undesired in my eyes, I would much rather have ...

I agree, that would be a neat feature. I'm working on adding shading to some extra plots- once that's done, maybe I'll investigate a fixed light source.

Would there be any way to make it consistent for the zero-extent bar as well?

That's a bug I'd rather not deal with yet that comes down to z-sorting coincident faces. We only got away with it before because the upper and lower faces were the same color. I'd prefer to leave that to a later PR, where the approach would be either:

• Sort by normal vector in the case of coincident faces
• Cull all faces with a back-pointing normal vector
  At least right now it looks correct from the top, which is probably the more important side.

Before I can reasonably do that, I'd like to get this and then #12136 in.

Why did the notshaded baseline image change?

Why did the notshaded baseline image change?

Because some faces are now rendered with the points in reverse order, and I guess the renderer somehow cares about that. The diff can be seen here:

There's a handful of changed pixels (shown in red) along the bottom axis, and the other edges.

Not sure why CI fails

Looks like it maybe just needs rebasing on top of a commit with CI configured. Shall I go ahead and do that?

@dstansby: Rebased

Was hoping you'd merge rather than squash to keep me from having to rebase. Oh well - thanks for putting this in anyway!

That setting is sticky, so I don't know if @timhoffm did that on purpose (I wouldn't have squashed for this PR). If it wasn't on purpose, please @timhoffm be careful on the next merge.

The stickiness has caught me out in the past too.

On the weekly call on 29 oct it was discussed that all PRs should be squashed-merged if possible to keep the history cleaner and reduce security risks. I think if someone does not want this to happen for their PR they should mention it.

Hmm, that probably should be announced. I'm also not particularly for it when it's an experienced contributor.

Sorry if squashing caused troubles. The changes seemed rather small and related so that I did not consider it important to keep separate commits.

+1 that an author should explicitly mention that commits should not be squashed.