Paths

Table of Contentst

Differential D2465

Adds optional fixed shadows without swimming effect
ClosedPublic
Actions

Authored by vladislavbelov on Dec 13 2019, 2:37 AM.

Details

Reviewers

Stan

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP23333: Adds optional fixed shadows without swimming effect

Summary

Fixed shadows have fixed rendering distance. Also they aren't swimming (blinking) on camera movement/rotation.

It can help players who want to see shadows but can run only low qualities. With fixed shadows players can set lower fixed distance. Also it increase performance for such cases, because less objects are rendered.

Test Plan

Apply the patch and compile the game
Run the game with fixed shadows enabled
Check that
- Fixed shadows work
- Low quality shadows look better with smaller fixed shadow distance
- On camera movement/rotation shadows don't swim/blink
- Fixed shadows aren't slower than usual shadows.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.Dec 13 2019, 2:37 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/747/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1263/display/redirect

Harbormaster completed remote builds in B10288: Diff 10574.Dec 13 2019, 2:45 AM

Silier awarded a token.Dec 13 2019, 7:45 AM

Stan added a subscriber: Stan.Dec 13 2019, 3:01 PM

Stan added inline comments.

binaries/data/config/default.cfg
76 ↗	(On Diff #10574)	No menu option?
source/renderer/ShadowMap.cpp
247 ↗	(On Diff #10574)	Cache `camera.GetOrientation()`?
252 ↗	(On Diff #10574)	Why not name them `first_base` `second_base` `height` Instead of adding a comment?
255 ↗	(On Diff #10574)	better name for `x`? Also it's shadowing the outer scope variable no?
263 ↗	(On Diff #10574)	Magic number?

vladislavbelov added inline comments.Dec 13 2019, 3:22 PM

source/renderer/ShadowMap.cpp
247 ↗	(On Diff #10574)	It's already cached, because it returns a const reference.
252 ↗	(On Diff #10574)	It makes sense (I just used these letters on paper x) ).
255 ↗	(On Diff #10574)	The same.
263 ↗	(On Diff #10574)	Nooo, it's insets x)

vladislavbelov added inline comments.Dec 13 2019, 5:48 PM

binaries/data/config/default.cfg
76 ↗	(On Diff #10574)	Not yet, but in future.

I tried the diff.
Shadows do not blink when moving camera.
Shadows with fixed distance 100 on low look amazing.

Patch is nice however it leads to weird artefacts in Atlas, maybe it could be a more gradual thing.

source/renderer/ShadowMap.cpp
255 ↗	(On Diff #10574)	"..\..\..\source\renderer\ShadowMap.cpp(255): warning C4456: declaration of 'x' hides previous local declaration [C:\Dev\OSS\trunk\build\workspaces\vc2015\graphics.vcxproj]"

Renames variables and removes name shadowing.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/767/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1283/display/redirect

Harbormaster completed remote builds in B10322: Diff 10605.Dec 15 2019, 12:23 AM

Stan requested changes to this revision.Dec 26 2019, 3:29 PM

Stan added inline comments.

source/renderer/ShadowMap.cpp
131 ↗	(On Diff #10605)	Question is the following code only setting the variable if it succeeds or does it set it to 0 if it fails? If so that line is useless.
175 ↗	(On Diff #10605)	Not a fan of this syntax but up to you.
178 ↗	(On Diff #10605)	Nuke braces Maybe invert condition?
245 ↗	(On Diff #10605)	Better name?

This revision now requires changes to proceed.Dec 26 2019, 3:29 PM

Fixes @Stan`s notes.

source/renderer/ShadowMap.cpp
131 ↗	(On Diff #10605)	If the name isn't present in the config the value doesn't change.

vladislavbelov marked an inline comment as done.Jan 4 2020, 5:47 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/946/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/42/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1464/display/redirect

Harbormaster completed remote builds in B10625: Diff 10875.Jan 4 2020, 5:53 PM

Looking alright my only concern now is it not being user friendly and as such not being used :)

binaries/data/config/default.cfg
69 ↗	(On Diff #10875)	Might be nice to unify all those variables in a shadow section one day. It's unrelated to this patch though.
76 ↗	(On Diff #10875)	It's a bit weird to have the comment cut in half... Do we need the trailing 0? I really think it should be enabled for low shadows. Else this option is not useful because it's unknown...
source/renderer/ShadowMap.cpp
131 ↗	(On Diff #10605)	Okay.

vladislavbelov added inline comments.Jan 5 2020, 12:40 AM

binaries/data/config/default.cfg
76 ↗	(On Diff #10875)	Actually is the on comment. Trailing `.0` says it's a floating pointer number. I'd prefer to add an option first. But before that I have something to do with shadows. So I didn't add fixed shadows for low settings.