Specular lighting fixes #2079

abaire · 2025-03-31T08:18:01Z

Various interdependent fixes for specular lighting.

The results generally don't exactly match HW but are much closer than the current implementation.

abaire · 2025-03-31T16:58:16Z

@Triticum0 any chance you could spot check this against games with known lighting issues? I am hopeful that it'll improve highlights in games using the fixed function pipeline.

re4thewin · 2025-03-31T19:33:34Z

this pr fixes 838

abaire · 2025-03-31T19:44:42Z

this pr fixes 838

Cool, thanks for testing!

Triticum0 · 2025-03-31T21:24:49Z

Not got at my computer for a while sorry. Will do some testing once I get back to my pc in few days

ko81e24wy · 2025-04-01T09:50:16Z

Also fixes #784, hard to say its same as XBOX,but much better than before.

abaire · 2025-04-01T14:25:29Z

Cool, thanks for testing!

The reconstruction of the specular power factor isn't perfect and there are still some issues with ambient/emissive handling in certain cases so I expect that there will be some delta vs HW with this fix.

ko81e24wy · 2025-04-02T12:27:13Z

I test NGB and DOA,this PR has stutter in game when master is smooth.

mborgerson · 2025-04-09T00:26:46Z

hw/xbox/nv2a/pgraph/pgraph.c

+    }
+
+    c3 -= SPECULAR_POWER_CONSTANT_COEFFICIENT;
+    float ret = SPECULAR_POWER_LOG_CONSTANT / log2(c3 / SPECULAR_POWER_COEFFICIENT_A);


Cross checked against this implementation. The power estimated by this function appears to have very high error for pow<1.0. Can you confirm?

It would be nice to determine the actual specular power formula hardware uses based on these parameters, but we can save it for future work.

It's also possible to calculate the exact input value given the observed original lookup table, but this curve fit approach is probably fine if accuracy is within a few percent given (2) anyway.

Interesting, given that it's disjoint in the LUT func I'm not surprised. It may not be possible to do a trivial curve fit, will look into it.

I ended up segmenting the curve and using a couple functions to keep percent error < 3. Things get wacky around power 2500 as params start moving in the opposite direction, but I would assume in practice power is typically < 1000 (and the same wackiness can be observed on HW).

The end results are still not quite correct; it looks like the localeye variants are quite close but the infinite ones are a bit overbright. I'm inclined to look into it in a followup since this PR adds a bunch of other functionality.

Ended up fixing the overbrightness from the point light -it seems that localeye off does not modify the VP vector in the way I'd assumed from the GL spec.

hw/xbox/nv2a/pgraph/pgraph.c

hw/xbox/nv2a/pgraph/glsl/vsh-ff.c

mborgerson · 2025-04-09T00:50:17Z

hw/xbox/nv2a/pgraph/pgraph.c

+    int slot = (method - NV097_SET_SPECULAR_PARAMS) / 4;
+    NV2A_DPRINTF("NV097_SET_SPECULAR_PARAMS[%d] 0x%X\n", slot, parameter);
+    if (slot == 3) {
+        pg->specular_power = reconstruct_specular_power_from_c3(parameter);


It would be nice to find where this is actually stored in device state

I assume it's not stored anywhere; presumably the 6 coefficients are stored since it wouldn't make sense to pass them only to have the HW reconstruct the original power value from DX/GL.

If someone reverses the actual implementation (the values look suspiciously similar to the ones used for NV097_SET_LIGHT_SPOT_FALLOFF which is also unimplemented) we could work out the registers with that?

presumably the 6 coefficients are stored

This is what I'm referring to

Unfortunately it doesn't seem to be a straightforward one. Unless the values are being transformed before storage (I'm asuming this is unlikely since they're floats rather than bitvectors) there doesn't seem to be any viable sets in the diff.

This is particularly annoying since it looks like the power can't be inferred from c3 alone; for vals < 1 c2 is pretty close on its own, but c2 rolls over around 1561.

I may just stash the values in PGRAPHState for now (since I need to accumulate them for curve fitting) and leave finding the correct locations to a future PR, if that's OK.

I may just stash the values in PGRAPHState for now (since I need to accumulate them for curve fitting) and leave finding the correct locations to a future PR, if that's OK.

No problem. We can iterate on it as we learn more. But as we discussed in your other comment, the new fields should be saved in the VMStateDescription

On second thought, let's not bother saving the new field in the VMStateDescription in this PR. Other fields are not saved right now, and we'll have to come up with a better solution for them. Games likely refreshes them anyway, so it's not a huge issue.

ko81e24wy · 2025-04-10T06:47:17Z

I test the latest build of this PR,crash when loading in game.
log file
xemu.zip

abaire · 2025-04-10T15:09:24Z

@ko81e24wy oops! I was setting up the uniform in the infinite light handler and that game has 3 infinite lights. Should be fixed now.

abaire · 2025-04-10T15:24:38Z

hw/xbox/nv2a/pgraph/pgraph.h

@@ -197,6 +197,11 @@ typedef struct PGRAPHState {
    float light_local_position[NV2A_MAX_LIGHTS][3];
    float light_local_attenuation[NV2A_MAX_LIGHTS][3];

+    float specular_params[6];


@mborgerson should these be added to VMStateDescription?

It might be useful to document the decision factor for adding pgraph state to snapshots (maybe with CI enforcement/reminders at some point, if it's easy to automate)

This is one reason to prefer PGRAPHState.regs over new fields in PGRAPHState. If they are stored in regs, they are already being stored in VMStateDescription. If not, then they need to be added specifically to the VMStateDescription.

The decision factor to include it in VMStateDescription is basically this: if the data are part of device state that cannot be calculated, then they must be in VMStateDescription for faithful state loading. Imagine if the game sets specular params once on startup and never does so again. If a snapshot is saved, then loaded later in a new instance of xemu, the restored state won't have correct specular params.

This is one reason to prefer PGRAPHState.regs over new fields in PGRAPHState.

Agreed on preferring regs, in this case I'm just not seeing any change in an obvious way.

... if the data are part of device state that cannot be calculated, then they must be in VMStateDescription

In that case I think there are a bunch of fields missing. E.g., light_infinite_half_vector and the other constants (likely more, I just happened to have an editor open looking at that line).

Would it make sense to either adopt a naming convention for vals that should (not?) be saved in vmstate or otherwise differentiate between recalculable and non fields? Then in theory we could have a check to make sure new fields update the state.

Or is the intent more nuanced since it's likely that many of the pgraph state fields will be set very frequently and it's acceptable to have a bad frame or two after a load?

Yeah, there are other fields missing. I mentioned in my comment above but I'll repeat here, I'm okay with deferring storing these new (and existing) fields VMStateDescription for now. Apologies for the misdirection.

ko81e24wy · 2025-04-10T15:43:34Z

@ko81e24wy oops! I was setting up the uniform in the infinite light handler and that game has 3 infinite lights. Should be fixed now.

It works now.

ko81e24wy · 2025-04-10T16:14:53Z

Performance is better,the cutsecne after boss fight stage1 in chapter 15 has no more frame drops like before.

abaire marked this pull request as ready for review March 31, 2025 19:44

This was referenced Mar 31, 2025

Pariah Graphical Issues #1241

Closed

Some games lighting effects dont render correctly. #586

Closed

nv2a 0x97 -> 0x03b8 (3D_SEPARATE_SPECULAR_ENABLE) unhandled #701

Closed

Constantine Graphical Issue #1201

Open

Pac-Man World 2 Shading Issues #1242

Open

This comment was marked as resolved.

Sign in to view

abaire force-pushed the feat_spec_params branch 2 times, most recently from bd6b911 to 39fef7e Compare April 2, 2025 02:11

This comment was marked as resolved.

Sign in to view

mborgerson mentioned this pull request Apr 2, 2025

Ninja gaiden black particle effects glitch #784

Closed

mborgerson reviewed Apr 9, 2025

View reviewed changes

nv2a: Partially handle SET_LIGHT_CONTROL

c4bf201

abaire force-pushed the feat_spec_params branch from 39fef7e to 51fdaca Compare April 10, 2025 05:20

abaire added 2 commits April 10, 2025 08:05

nv2a: Partial implementation of SET_SPECULAR_PARAMS

4474754

nv2a: Handle LOCAL_RANGE

83e8f65

abaire force-pushed the feat_spec_params branch from 51fdaca to c9c970e Compare April 10, 2025 15:06

abaire commented Apr 10, 2025

View reviewed changes

nv2a: Handle LOCALEYE light control

0e8fdba

abaire force-pushed the feat_spec_params branch from c9c970e to 0e8fdba Compare April 10, 2025 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specular lighting fixes #2079

Specular lighting fixes #2079

abaire commented Mar 31, 2025 •

edited by mborgerson

Loading

abaire commented Mar 31, 2025

re4thewin commented Mar 31, 2025

abaire commented Mar 31, 2025

Triticum0 commented Mar 31, 2025

ko81e24wy commented Apr 1, 2025 •

edited

Loading

abaire commented Apr 1, 2025

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

ko81e24wy commented Apr 2, 2025

mborgerson Apr 9, 2025

abaire Apr 9, 2025

abaire Apr 10, 2025

abaire Apr 10, 2025

mborgerson Apr 9, 2025

abaire Apr 9, 2025

mborgerson Apr 9, 2025

abaire Apr 9, 2025

mborgerson Apr 10, 2025

mborgerson Apr 11, 2025

ko81e24wy commented Apr 10, 2025 •

edited

Loading

abaire commented Apr 10, 2025

abaire Apr 10, 2025

mborgerson Apr 10, 2025 •

edited

Loading

abaire Apr 10, 2025

mborgerson Apr 11, 2025

ko81e24wy commented Apr 10, 2025

ko81e24wy commented Apr 10, 2025

Specular lighting fixes #2079

Are you sure you want to change the base?

Specular lighting fixes #2079

Conversation

abaire commented Mar 31, 2025 • edited by mborgerson Loading

abaire commented Mar 31, 2025

re4thewin commented Mar 31, 2025

abaire commented Mar 31, 2025

Triticum0 commented Mar 31, 2025

ko81e24wy commented Apr 1, 2025 • edited Loading

abaire commented Apr 1, 2025

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

ko81e24wy commented Apr 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ko81e24wy commented Apr 10, 2025 • edited Loading

abaire commented Apr 10, 2025

Choose a reason for hiding this comment

mborgerson Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ko81e24wy commented Apr 10, 2025

ko81e24wy commented Apr 10, 2025

abaire commented Mar 31, 2025 •

edited by mborgerson

Loading

ko81e24wy commented Apr 1, 2025 •

edited

Loading

ko81e24wy commented Apr 10, 2025 •

edited

Loading

mborgerson Apr 10, 2025 •

edited

Loading