GLES2 2D fix normal mapping - batching and nvidia workaround

[lawnjelly]

Normal mapping previously took no account of rotation or flips in any path except the TEXTURE_RECT (uniform draw) method. This passed flips to the shader in uniforms.

In order to pass flips and rotations to the shader in batching and nvidia workaround, a per vertex attribute is required rather than a uniform. This introduces LIGHT_ANGLE which encodes both the rotation of a quad (vertex) and the horizontal and vertical flip.

In order to optionally store light angles in batching, we switch to using a 'unit' sized array which can be reused for different FVF types, as there is no need for a separate array for each FVF, as it is a waste of memory.

Fixes #41324.
Related to #41254 and #40905 (same problem in GLES3).

Notes

• This is a relatively large change in the batching, it should be tested (probably alongside GLES3 batching?) probably for 3.2.4.

[lawnjelly]

This works in my tests, but it would be nice to test it with some normal mapped 2d games. If anyone could test it, or point my in the direction of 2d demos or open source games I can download and try that would be very useful. 👍

[Calinou]

I made a PR to add normal maps to the 2D lights and shadows demo: godotengine/godot-demo-projects#511

[lawnjelly]

Just WIPing these 2 PRs temporarily as testing with @Calinou's demo has revealed there is another way of flipping sprites aside from setting flip in the offset section - you can set the scale to -1 in x or y. So I need to account for this.

EDIT : Ok touch wood, I think that the negative scaling flipping is sorted. It is working in all my tests now and in @Calinou's demo.

The negative scaling seems to work as is with the nvidia_workarounds, because the transform is sent regularly to the GPU. However with batching, it interferes with the calculation for the light angle, which assumes positive scaling for x and y.

The solution is to test the cross product of the x and y axes from the transform basis, if they are negative, then negative scaling has affected the light angle, and we apply a vertical flip in the shader.

If that all sounds hard to follow, that's understandable, it is quite a complex indirect path to get the correct angle in the shader! 😁 There isn't an obvious simpler way around this that I can think of, but it should work pretty well.

[clayjohn]

I've read through the PR and it looks pretty good. At first I was a little wary of the templating, but It looks like you have kept it as simple as possible.

We need to wait until after 3.2.3 releases before we even consider merging this though. While it does fix a bug, it is a high risk PR and should not be considered for a RC release.

Great work! :)

[lawnjelly]

I've read through the PR and it looks pretty good. At first I was a little wary of the templating, but It looks like you have kept it as simple as possible.

I'll double check with some tests of the templating in a single file test case for the compiler outside of Godot, looking at the assembly output, to check it works as expected. There are potentially alternative ways of achieving the same thing e.g.:

• normal code may result in 2 independent versions of the function in some compilers, with some compiler options (perhaps depending whether it is in the header or cpp), in some situations
• FORCE_INLINE
• Macros

I'm currently of the view that templates are probably the best option (if they can work - sometimes macros may be needed, templates don't work well for certain scenarios). Templates are explicit to the compiler (don't rely on particular compiler behaviour), should work in debug, and don't pollute the namespace like macros.

Downsides to templates can be boiler plate / how easy they are to read, but in this case, it should be very obvious.

FORCE_INLINE can potentially be used, but our intention is not to inline these functions, but to ensure there are two separate versions in the executable. FORCE_INLINE is subject to the problem that it can easily cause many more versions of the function being embedded than were originally intended (due to loop unrolling etc).

[lawnjelly]

Closing this for now as it should be part of unified batching.

[akien-mga]

@lawnjelly Well if this was ready, I think it's best to merge this part first to have it tested. I'll probably have a 3.2.4 beta 1 out soon to test what was already merged before you're done with #42119.

[lawnjelly]

@lawnjelly Well if this was ready, I think it's best to merge this part first to have it tested. I'll probably have a 3.2.4 beta 1 out soon to test what was already merged before you're done with #42119.

I'll leave it up to you, there could be some useful feedback by merging it for the first beta I agree.

The unified batching is almost ready for a beta too, but unfortunately I am away this week (not allowed to have fun programming all the time!), and spent most of yesterday fixing static checks and compiler warnings - there are quite a few to sort out. Plus I'm sure it will need a few naming adjustments and clayjohn to review, which will be no small task as it is 4-5k new lines of code. 😁

[akien-mga]

Makes sense, let's merge this one for now then.

Enjoy some time off screen :)

[akien-mga]

Thanks!