Per pixel lighting support in Metal #1551

colincornaby · 2024-01-08T00:25:38Z

This PR adds Metal per pixel lighting support to the shaders - controlled by a preprocessor flag. Per pixel lighting defaults to off in since it's an alternative rendering mode that does not match the original engine. While per pixel lighting seems correct - it should be regarded as experimental. Per pixel shading enables a shader variant for the "fixed" pipeline shader. This shader variant is controlled by a shader compile time flag. However - this flag is a function constant which means evaluation of it can be deferred to when the shader is compiled from Metal byte code into vender specific assembly. I.E. - Metal can wait to apply the flag until the final runtime shader compilation.

Metal was originally ported from the OpenGL fork which used per pixel lighting. Metal switched back to per vertex for several reasons:

The fragment shader function was trying to emulate the Direct3D fixed function pipeline - and because of high register usage was running into a lot of register overflows. The additional registers required by per pixel rendering was causing extreme performance issues. The shader has moved most of this register usage into compile time constants - so there are enough registers free now to do per pixel lighting.
Most of Uru uses lower complexity meshes or light maps. Bump mapping was never in Uru. So the benefits of per pixel lighting were less - and per pixel lighting was deprioritized.

Please look over this closely - I want to make sure the per vertex path has remained unchanged. Not in a rush to get this in.

colincornaby · 2024-01-08T00:29:50Z

Sources/Plasma/FeatureLib/pfMetalPipeline/ShaderSrc/FixedPipelineShaders.metal

@@ -445,7 +472,7 @@ fragment half4 pipelineFragmentShader(ColorInOut in [[stage_in]],
            }
        }

-        currentColor = half4(in.vtxColor.rgb, 1.0h) * currentColor;


Might be a good time to ask about this one. After blending the layers - the shader again multiplies by the vertex +lighting color. This was carried over from the original GL shaders. I know if this isn't here the output is wrong. But I'm not sure why.

Skimming through the code, my guess is this:
currentColor starts out set to the lighting colour with no material values, but then on the first layer is overwritten to the layer colours of the first layer (in blendFirst) with subsequent layers blending into that. Since we've lost the lighting colours at that point, we need to multiply them back in.

colincornaby · 2024-06-02T06:22:18Z

I brought this branch up to date with master - and now it's acting strangely. For example - it's hitting asserts while trying to load the server.ini. I'll dig a bit and see whats going on. I'm pretty sure this behavior is newly unique to this branch. Last time I was in here it was working.

colincornaby · 2024-06-02T20:28:53Z

The issue I was befuddled by is addressed in #1591.

dpogue · 2024-06-02T22:45:31Z

FWIW I'm pretty sure OpenGL is also doing per-vertex lighting to match DirectX.

I might have been doing that in the fragment shader at one point while I was getting it set up, but it's in the vertex shader in the current version of the GL branch.

colincornaby · 2024-06-16T21:10:56Z

Re-stating discussions elsewhere here for the status on this branch.

While evaluating the lighting pipeline - it was found Metal is not properly handling the bump maps.
Plasma calls them "bump maps" but they actually seem to be normal maps.
Normal maps typically require per pixel lighting. In the D3D renderer there is a algorithm that attempts to bake the lighting algorithm into the texturing pipeline.
This algorithm has significant issues - but would have been a reasonable approximation in the early 2000s.

Relative to this PR:

We're not going to reimplement Cyan's algorithm in Metal. We have the ability to do proper normal mapping on Metal hardware.
We will not ship per pixel lighting by default - but meshes can opt into it.
Meshes with normal mapping will be opted into per pixel lighting. A normal mapping path in the shaders will be implemented in a future PR.
The Metal pipeline will render normal maps differently than Windows - but the quality should be much higher.

We'll want to move ahead with this PR to get normal mapping into Metal.

colincornaby · 2024-08-04T20:11:02Z

@dpogue - Checking back in to see if you have any thoughts here. We'll need this for bump mapping (even if we don't turn it on for everything.) Not high priority, just didn't want it to get lost.

dpogue

Overall I think this is okay. Admittedly I only skimmed the actual shader code implementation details.

I think we should do the work now to refactor this a bit so that it can be turned on/off dynamically per mesh, even if we don't actually hook up the dynamic choice yet.

Otherwise, just a few code style issues.

dpogue · 2024-08-06T08:55:21Z

Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipeline.cpp

+    if (PLASMA_PER_PIXEL_LIGHTING)
+    {
+        fDevice->CurrentRenderCommandEncoder()->setFragmentBytes(&fDevice->fPipeline->fCurrentRenderPassMaterialLighting, sizeof(plMaterialLightingDescriptor), FragmentShaderArgumentMaterialLighting);
+    }


Doesn't match our coding style, but we probably also want to make this controlled by a pipeline value rather than a define (even if we just hardcode that pipeline value to the value of the define for now)

dpogue · 2024-08-06T08:55:29Z

Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipeline.cpp

+    if (PLASMA_PER_PIXEL_LIGHTING)
+    {
+        fDevice->CurrentRenderCommandEncoder()->setFragmentBytes(lights, sizeof(plMetalLights), FragmentShaderArgumentLights);
+    } else {
+        fDevice->CurrentRenderCommandEncoder()->setVertexBytes(lights, sizeof(plMetalLights), VertexShaderArgumentLights);
+    }


Doesn't match our coding style, but we probably also want to make this controlled by a pipeline value rather than a define (even if we just hardcode that pipeline value to the value of the define for now)

dpogue · 2024-08-06T08:57:11Z

Sources/Plasma/FeatureLib/pfMetalPipeline/ShaderSrc/FixedPipelineShaders.metal

+    if (perPixelLighting)
+    {
+        // send the world pos on to the pixel shader for lighting
+        out.worldPos = position;
+        out.normal = Ndirection;
+    }
+
+    if (perPixelLighting)
+    {
+        out.vtxColor = inColor;
+    } else {
+        out.vtxColor = calcLitMaterialColor(lights, inColor, materialLighting, position, Ndirection);
+    }


I don't know how strict our coding style has been within Metal shaders, but the opening brace should be on the same line as the if

Because Metal Shaders are C++ - I'm assuming we should apply the same coding style.

dpogue · 2024-08-06T08:58:26Z

Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipelineState.cpp

+    bool perPixelLighting = PLASMA_PER_PIXEL_LIGHTING;
+    constants->setConstantValue(&perPixelLighting, MTL::DataTypeBool, FunctionConstantPerPixelLighting);


TODO: move the calculation of this to the pipeline so that it can (later) be dynamic per mesh

Hoikas

Needs code style cleanups

Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipeline.cpp

Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipelineState.cpp

Also cleaning up some redundant state binding

colincornaby · 2024-11-18T06:59:02Z

With the latest changes I added some better detection of redundant bindings. I can see the redundant bindings in the Metal debugger, and it's been on my list of things to address. Unnecessary draw commands create a larger buffer to send to the GPU - so they should be avoided.

However - right now I'm using memcmp to compare struct values. I'd rather use a bool that marks when something is dirty and needs to be sent to the GPU. I've left notes behind in the code and will continue improving this in the future.

colincornaby commented Jan 8, 2024

View reviewed changes

colincornaby force-pushed the metal-per-pixel-lighting branch from 8ce854b to 9f88aea Compare March 31, 2024 04:09

colincornaby force-pushed the metal-per-pixel-lighting branch from 9f88aea to 315ef75 Compare June 2, 2024 04:24

colincornaby requested a review from dpogue June 2, 2024 04:54

dpogue reviewed Aug 6, 2024

View reviewed changes

colincornaby force-pushed the metal-per-pixel-lighting branch from 315ef75 to 5ca8e69 Compare August 11, 2024 03:22

colincornaby added 3 commits November 15, 2024 22:59

Initial Metal lighting calc cleanup

64f1b63

Per pixel lighting option in Metal

e0edcd0

Aligning per pixel lighting shader with code style

36c1fb8

colincornaby force-pushed the metal-per-pixel-lighting branch from 0be27a0 to 36c1fb8 Compare November 16, 2024 06:59

Hoikas reviewed Nov 16, 2024

View reviewed changes

colincornaby added 2 commits November 17, 2024 22:53

Allowing per pixel lighting to be turned on dynamically

dbd10b8

Also cleaning up some redundant state binding

Code style fixes

9d6cf8d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per pixel lighting support in Metal #1551

Per pixel lighting support in Metal #1551

colincornaby commented Jan 8, 2024 •

edited

Loading

colincornaby Jan 8, 2024

dpogue Jun 2, 2024

colincornaby commented Jun 2, 2024

colincornaby commented Jun 2, 2024

dpogue commented Jun 2, 2024

colincornaby commented Jun 16, 2024

colincornaby commented Aug 4, 2024

dpogue left a comment

dpogue Aug 6, 2024

dpogue Aug 6, 2024

dpogue Aug 6, 2024

colincornaby Aug 7, 2024

dpogue Aug 6, 2024

Hoikas left a comment

colincornaby commented Nov 18, 2024

		bool perPixelLighting = PLASMA_PER_PIXEL_LIGHTING;
		constants->setConstantValue(&perPixelLighting, MTL::DataTypeBool, FunctionConstantPerPixelLighting);

Per pixel lighting support in Metal #1551

Are you sure you want to change the base?

Per pixel lighting support in Metal #1551

Conversation

colincornaby commented Jan 8, 2024 • edited Loading

colincornaby Jan 8, 2024

Choose a reason for hiding this comment

dpogue Jun 2, 2024

Choose a reason for hiding this comment

colincornaby commented Jun 2, 2024

colincornaby commented Jun 2, 2024

dpogue commented Jun 2, 2024

colincornaby commented Jun 16, 2024

colincornaby commented Aug 4, 2024

dpogue left a comment

Choose a reason for hiding this comment

dpogue Aug 6, 2024

Choose a reason for hiding this comment

dpogue Aug 6, 2024

Choose a reason for hiding this comment

dpogue Aug 6, 2024

Choose a reason for hiding this comment

colincornaby Aug 7, 2024

Choose a reason for hiding this comment

dpogue Aug 6, 2024

Choose a reason for hiding this comment

Hoikas left a comment

Choose a reason for hiding this comment

colincornaby commented Nov 18, 2024

colincornaby commented Jan 8, 2024 •

edited

Loading