Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dynamic lights intensity, cleanup #1425

Merged
merged 7 commits into from
Jan 28, 2025

Conversation

VReaperV
Copy link
Contributor

@VReaperV VReaperV commented Nov 9, 2024

Cgame-side pr: Unvanquished/Unvanquished#3184
Dynamic light config update for weapon flash colours: UnvanquishedAssets/res-weapons_src.dpkdir#32

  • NUKED the weird 4x dynamic light scaling
  • NUKED the weird r_lightScale cvar, which is used for dynamic lights, but it's a cheat cvar and always set to 2.0, and essentially only used for dynamic lights spawned from movers
  • NUKED RE_AddDynamicLightToSceneQ3A() and renamed RE_AddDynamicLightToSceneET() to RE_AddDynamicLightToScene() because they're essentially the same
  • NUKED now unused trap_R_AddAdditiveLightToScene()
  • NUKED light intensity/scale, which was only used to scale the light's colour, just multiply the colour earlier on instead
  • Fixed some incorrect memory read before bounds check

Fixes #61:
hq-beta28:
Before:
unvanquished_2024-11-09_184524_000
After:
unvanquished_2024-11-09_184725_000

tremtest:
Before:
unvanquished_2024-11-09_184614_000
After:
unvanquished_2024-11-09_184754_000

@slipher
Copy link
Member

slipher commented Nov 10, 2024

Why don't we nuke the intensity argument? It seems to be redundant with just changing the r/g/b values.

@VReaperV VReaperV force-pushed the reaper/fix-dynamic-lights-intensity/sync branch from b628f0a to 0670349 Compare November 10, 2024 08:26
@VReaperV
Copy link
Contributor Author

Why don't we nuke the intensity argument? It seems to be redundant with just changing the r/g/b values.

Right, done now.

@VReaperV VReaperV force-pushed the reaper/fix-dynamic-lights-intensity/sync branch from 50875cb to ffb8083 Compare November 10, 2024 08:57
light->l.scale = -intensity;
else
light->l.scale = intensity;
if ( light->l.inverseShadows ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The forward lighting shader checks the sign of u_LightScale to check for inverse lights. These are said to be already broken, but it would be nice to set the scale to 1 or -1 to at least gesture at how it was supposed to work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scale is already removed there, so there's nothing to set.

@slipher
Copy link
Member

slipher commented Nov 15, 2024

I tested this branch vs. master and found the dynamic lights are not as bright. I guess if you deleted a factor of 8 of random scaling, you need to add that back in somewhere to get the same result?

Master:
unvanquished-plat23-dlight-psaw

This branch:
unvanquished-plat23-dlight-psaw

@VReaperV VReaperV force-pushed the reaper/fix-dynamic-lights-intensity/sync branch from ffb8083 to 3993866 Compare January 5, 2025 13:11
VReaperV added a commit to UnvanquishedAssets/res-weapons_src.dpkdir that referenced this pull request Jan 5, 2025
DaemonEngine/Daemon#1425 NUKEs random light scaling, this sets the dynamic light values to compensate for that.
@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 5, 2025

I tested this branch vs. master and found the dynamic lights are not as bright. I guess if you deleted a factor of 8 of random scaling, you need to add that back in somewhere to get the same result?

Master: unvanquished-plat23-dlight-psaw

This branch: unvanquished-plat23-dlight-psaw

I have created a PR in res-weapons repo for this: UnvanquishedAssets/res-weapons_src.dpkdir#32.

Copy link
Contributor

@DolceTriade DolceTriade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested ingame and feels good

@slipher
Copy link
Member

slipher commented Jan 6, 2025

There are some maps with particle systems with dynamic lights, so not everything can be made the same by only adjusting the game’s own assets.

Also there are some places where dynamic light intensities from config files are clamped to [0, 1] (yeah that’s stupid and shouldn’t be done), so that’s something we would need to watch out for if adjusting configs.

@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 6, 2025

There are some maps with particle systems with dynamic lights, so not everything can be made the same by only adjusting the game’s own assets.

Which maps?

Also there are some places where dynamic light intensities from config files are clamped to [0, 1] (yeah that’s stupid and shouldn’t be done), so that’s something we would need to watch out for if adjusting configs.

Yes, that is particles and beams/trails. Those look fine though.

@slipher
Copy link
Member

slipher commented Jan 9, 2025

There are some maps with particle systems with dynamic lights, so not everything can be made the same by only adjusting the game’s own assets.

Which maps?

For example usstremor at viewpos 112 -306 86 6 2. Here is an index where all map particle systems with one can be found under dynamiclight. (Map trail systems could use them too, but none do.)

Also there are some places where dynamic light intensities from config files are clamped to [0, 1] (yeah that’s stupid and shouldn’t be done), so that’s something we would need to watch out for if adjusting configs.

Yes, that is particles and beams/trails. Those look fine though.

We have particle and trail systems using dynamic lights in res-weapons, but I don't see any updates for them in UnvanquishedAssets/res-weapons_src.dpkdir#32.

Why don't we just add back the scaling factors to anywhere the lights are used on the gamelogic side? This is an easy way to avoid unwanted changes to assets. I doubt designers are coming in with a specific expectation like 'the illumination should be equivalent to fullbright at distance 200 with intensity 17.4', so the arbitrary factor doesn't hurt anything.

@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 9, 2025

For example usstremor at viewpos 112 -306 86 6 2. Here is an index where all map particle systems with one can be found under dynamiclight. (Map trail systems could use them too, but none do.)

Thanks, I'll test those once I update this/Unvanquished-side branch.

We have particle and trail systems using dynamic lights in res-weapons, but I don't see any updates for them in UnvanquishedAssets/res-weapons_src.dpkdir#32.

I tried with those initially, but they ended up looking wrong because of the 8-bit precision limit.

Why don't we just add back the scaling factors to anywhere the lights are used on the gamelogic side? This is an easy way to avoid unwanted changes to assets. I doubt designers are coming in with a specific expectation like 'the illumination should be equivalent to fullbright at distance 200 with intensity 17.4', so the arbitrary factor doesn't hurt anything.

I agree, I've been thinking about doing just that.

@VReaperV VReaperV force-pushed the reaper/fix-dynamic-lights-intensity/sync branch from 3993866 to d6d0194 Compare January 26, 2025 01:54
This was a cheat cvar used for dynamic lights that have no intensity set, and set to 2.0 for absolutely no reason whatsoever.
…ET() to RE_AddDynamicLightToScene()

These were largely doing the same thing, use just one function instead.
This has largely the same functionality as trap_R_AddLightToScene().
This was only used to scale the light's colour, just multiply the colour with it earlier on instead.
@VReaperV VReaperV force-pushed the reaper/fix-dynamic-lights-intensity/sync branch from d6d0194 to 2585690 Compare January 26, 2025 01:55
@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 26, 2025

We have particle and trail systems using dynamic lights in res-weapons, but I don't see any updates for them in UnvanquishedAssets/res-weapons_src.dpkdir#32.

Why don't we just add back the scaling factors to anywhere the lights are used on the gamelogic side? This is an easy way to avoid unwanted changes to assets. I doubt designers are coming in with a specific expectation like 'the illumination should be equivalent to fullbright at distance 200 with intensity 17.4', so the arbitrary factor doesn't hurt anything.

I've added a fix for that to the Unvanquished-side pr. Both the dynamic lights on maps and those produced by weapons look fine to me with that change.

@VReaperV VReaperV merged commit 6538ceb into for-0.56.0/sync Jan 28, 2025
9 checks passed
VReaperV added a commit to UnvanquishedAssets/res-weapons_src.dpkdir that referenced this pull request Jan 28, 2025
DaemonEngine/Daemon#1425 NUKEs random light scaling, this sets the dynamic light values to compensate for that.
@VReaperV VReaperV deleted the reaper/fix-dynamic-lights-intensity/sync branch January 28, 2025 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Very bright dynamic lights on some maps
3 participants