Page MenuHomeWildfire Games

Clean up renderer options
ClosedPublic

Authored by wraitii on May 28 2019, 10:54 AM.

Details

Reviewers
historic_bruno
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22610: Refactor renderer options.
Summary

This refactors the renderer options into their own class. The point is ultimately to:

  • allow one to only include the rendering options, not the whole renderer header, when one wants access to rendering options.
  • centralise rendering changes and their side-effects.
  • clean up code.
Test Plan

Compile, check in-game that things aren't broken.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
temp
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8584
Build 14055: Vulcan BuildJenkins
Build 14054: arc lint + arc unit

Event Timeline

wraitii created this revision.May 28 2019, 10:54 AM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.May 28 2019, 10:54 AM

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

Linter detected issues:
Executing section Source...

source/graphics/LOSTexture.cpp
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2014"

source/simulation2/components/CCmpUnitRenderer.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/graphics/Model.cpp
|   1| /*·Copyright·(C)·2016·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2016"

source/ps/GameSetup/Config.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/renderer/DecalRData.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

source/renderer/scripting/JSInterface_Renderer.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/tools/atlas/GameInterface/View.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/tools/atlas/GameInterface/View.cpp
| 213| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/renderer/PatchRData.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/renderer/Renderer.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/tools/atlas/GameInterface/ActorViewer.cpp
| 213| »   void·UpdatePropList();
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/gui/MiniMap.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/graphics/MaterialManager.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/ps/GameSetup/Config.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/renderer/ParticleRenderer.cpp
|   1| /*·Copyright·(C)·2011·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2011"

source/renderer/scripting/JSInterface_Renderer.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/renderer/ShadowMap.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/renderer/OverlayRenderer.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1524/display/redirect

I haven't build- or run-tested this yet, but the diff looks conceptually OK.

See inline comments.

source/graphics/LOSTexture.cpp
1

2019

source/graphics/MaterialManager.cpp
1

2019

source/graphics/Model.cpp
1

2019

source/gui/MiniMap.cpp
1

2019

source/ps/GameSetup/Config.cpp
1

2019

source/ps/GameSetup/Config.h
1

2019

source/renderer/Renderer.h
1

2019

source/renderer/ShadowMap.cpp
1

2019

source/renderer/WaterManager.cpp
115

I don't understand the removal of an assignment here.

source/renderer/scripting/JSInterface_Renderer.cpp
1

2019

source/renderer/scripting/JSInterface_Renderer.h
1

2019

source/simulation2/components/CCmpUnitRenderer.cpp
1

2019

source/tools/atlas/GameInterface/View.cpp
1

2019

historic_bruno requested changes to this revision.Jul 20 2019, 9:43 PM
This revision now requires changes to proceed.Jul 20 2019, 9:43 PM
wraitii updated this revision to Diff 9036.Jul 21 2019, 5:08 PM

Rebased, clean up years, add back init.

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

Linter detected issues:
Executing section Source...

source/renderer/Renderer.h
|  69| class·CRenderer·:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCRenderer:' is invalid C code. Use --std or --language to configure the language.

source/renderer/scripting/JSInterface_Renderer.h
|  27| namespace·JSI_Renderer
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceJSI_Renderer{' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/ActorViewer.cpp
| 211| »   ·*·Recursively·fetches·the·props·of·the·currently·displayed·entity·model·and·its·submodels,·and·stores·them·for·rendering.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/View.cpp
| 211| »   »   g_Game->Interpolate(actualFrameLength,·realFrameLength);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

wraitii updated this revision to Diff 9039.Jul 21 2019, 6:19 PM

Forgot to remove code from D1926, also silence the warning.

Build failure - The Moirai have given mortals hearts that can endure.

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

wraitii updated this revision to Diff 9040.Jul 21 2019, 6:29 PM

Actually remove the code from D1926 this time :p

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

Linter detected issues:
Executing section Source...

source/renderer/scripting/JSInterface_Renderer.h
|  27| namespace·JSI_Renderer
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceJSI_Renderer{' is invalid C code. Use --std or --language to configure the language.

source/renderer/Renderer.h
|  69| class·CRenderer·:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCRenderer:' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/ActorViewer.cpp
| 211| »   ·*·Recursively·fetches·the·props·of·the·currently·displayed·entity·model·and·its·submodels,·and·stores·them·for·rendering.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/View.cpp
| 211| »   »   g_Game->Interpolate(actualFrameLength,·realFrameLength);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

historic_bruno requested changes to this revision.EditedJul 22 2019, 9:56 PM

From IRC discussion 20190721, two issues found:

  • Toggling Post processing option in-game causes an error
  • Toggling shadows on Azure Coast 3 scenario products strange artifacts (not present on SVN or A23b):

https://i.imgur.com/qGpSnVl.jpg

This revision now requires changes to proceed.Jul 22 2019, 9:56 PM
wraitii updated this revision to Diff 9100.Jul 24 2019, 5:29 PM

Had renamed Postproc to PostProc hence the error, and simply forgot to call MakeShadersDirty so obviously shadows didn't work.

Now fixed, thanks for testing.

Build failure - The Moirai have given mortals hearts that can endure.

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

historic_bruno requested changes to this revision.Jul 24 2019, 7:14 PM

From IRC discussion 20190721, two issues found:

  • Toggling Post processing option in-game causes an error
  • Toggling shadows on Azure Coast 3 scenario products strange artifacts (not present on SVN or A23b):

https://i.imgur.com/qGpSnVl.jpg

Confirmed both problems fixed. Config and HWDetect also tested.

source/renderer/scripting/JSInterface_Renderer.cpp
92

These should go in RenderingOptions as a part of an OPTION_WITH_SIDE_EFFECT, right? If these functions get called elsewhere in the engine, we have the same problem. Looks like Actor Viewer does call SetShadows. (Would have to make sure this doesn't break any of the other call sites like InitRenderer, though)

This revision now requires changes to proceed.Jul 24 2019, 7:14 PM
wraitii added inline comments.Jul 24 2019, 7:21 PM
source/renderer/scripting/JSInterface_Renderer.cpp
92

Ar, you're quite right indeed, this should go in RenderingOptions::SetShadows directly.

wraitii updated this revision to Diff 9108.Jul 24 2019, 7:30 PM

Do the side effects in RenderingOptions instead of the JS, which means I don't have to make RecomputeSystemShaderDefines public and that's much better.

source/renderer/scripting/JSInterface_Renderer.cpp
92

I actually completely forgot how my own patch worked there :P thanks for the notice.

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

Linter detected issues:
Executing section Source...

source/renderer/scripting/JSInterface_Renderer.h
|  27| namespace·JSI_Renderer
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceJSI_Renderer{' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/ActorViewer.cpp
| 211| »   ·*·Recursively·fetches·the·props·of·the·currently·displayed·entity·model·and·its·submodels,·and·stores·them·for·rendering.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/renderer/Renderer.h
|  69| class·CRenderer·:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCRenderer:' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/View.cpp
| 211| »   »   //·not·in·every·call·to·g_Game->Update
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

historic_bruno added inline comments.Jul 25 2019, 8:55 AM
source/renderer/RenderingOptions.h
27

Why the change from CStr to CStr8?

wraitii added inline comments.Jul 25 2019, 10:30 AM
source/renderer/RenderingOptions.h
27

CStr isn't a type. It's a define that takes CStr8 and CStrW alternatively, we include the file twice
(I'm not _entirely_ sure why it was done that way instead of using templates tbh - might just be too old).

So it just didn't work iirc.

historic_bruno added inline comments.Jul 25 2019, 8:15 PM
source/renderer/RenderingOptions.h
27

I think it only temporarily becomes CStrW for the purpose of compiling a shared source file. Everywhere else, CStr should be the same as a CStr8 (although it's a macro). There are actually very few places in the code where CStr8 is used compared to CStr or CStrW. It's also discouraged in the coding conventions: https://trac.wildfiregames.com/wiki/Coding_Conventions#Strings

Unless the goal was to not include CStr.h here?

wraitii added inline comments.Jul 25 2019, 9:54 PM
source/renderer/RenderingOptions.h
27

The goal was probably to not include CStr.h there.

source/renderer/RenderingOptions.h
27

OK, that seems to be how it's used elsewhere (sparingly), makes sense.

@historic_bruno I imagine you'll give this another to go accept the diff?

source/renderer/RenderingOptions.h
27

I've become quite wary of compilation times, and including headers everywhere is a serious problem.

So I would actually motion to change our coding conventions on this.

Nu clue about the code, but I tested (change options, restart reveal map and back) it on several random maps and found no problems.

wraitii updated this revision to Diff 9140.Jul 27 2019, 4:33 PM

Don't move code around, RC

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

Linter detected issues:
Executing section Source...

source/renderer/Renderer.h
|  69| class·CRenderer·:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCRenderer:' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/View.cpp
| 211| »   »   //·not·in·every·call·to·g_Game->Update
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/renderer/scripting/JSInterface_Renderer.h
|  27| namespace·JSI_Renderer
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceJSI_Renderer{' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/ActorViewer.cpp
| 211| »   ·*·Recursively·fetches·the·props·of·the·currently·displayed·entity·model·and·its·submodels,·and·stores·them·for·rendering.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

This revision was not accepted when it landed; it landed in state Needs Review.Aug 4 2019, 10:28 AM
Closed by commit rP22610: Refactor renderer options. (authored by wraitii). · Explain Why
This revision was automatically updated to reflect the committed changes.