Page MenuHomeWildfire Games

Contrast-Adaptiv-Sharpening pass
ClosedPublic

Authored by OptimusShepard on Feb 20 2020, 8:50 PM.

Details

Summary

Adds an sharpening pass option to the game.

Test Plan

Select CAS as sharpening pass in the graphic settings. Select different sharpness factors by slider.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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/PostprocManager.h
|  26| class·CPostprocManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPostprocManager{' is invalid C code. Use --std or --language to configure the language.

source/ps/CStrInternStatic.h
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2014"
Executing section JS...
Executing section cli...

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

Add an editing info to the cas.fs header.

OptimusShepard marked 2 inline comments as done.Feb 26 2020, 3:59 PM
OptimusShepard added inline comments.
binaries/data/mods/public/shaders/glsl/cas.fs
3 ↗(On Diff #11428)

I compared the original source and the Reshade port. It's nearly the same. They only difference I've adopted is, to put the scalar operations together to a vector. I don't think this is license relevant?

3 ↗(On Diff #11428)

I added an commentary to the end of the header.

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

Linter detected issues:
Executing section Source...

source/renderer/PostprocManager.h
|  26| class·CPostprocManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPostprocManager{' is invalid C code. Use --std or --language to configure the language.

source/ps/CStrInternStatic.h
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2014"

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.
Executing section JS...
Executing section cli...

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

nani added a subscriber: nani.Feb 26 2020, 4:45 PM
nani added inline comments.
binaries/data/mods/public/shaders/glsl/cas.fs
51 ↗(On Diff #11432)

Some compilers don't infer the value type so better change vec2(1, 1) to vec2(1.0, 1.0).
Do the same for all the other values meant to be a float.

Change the representation of floats in cas.fs.

OptimusShepard marked an inline comment as done.Feb 26 2020, 6:51 PM

Thx

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

Linter detected issues:
Executing section Source...

source/renderer/PostprocManager.h
|  26| class·CPostprocManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPostprocManager{' is invalid C code. Use --std or --language to configure the language.

source/ps/CStrInternStatic.h
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2014"

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.
Executing section JS...
Executing section cli...

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

My friend, would you consider updating something?

The patch is waiting for reviewing, what update do you want to have? The patch is mostly working. As it have the same problem as the FXAA patch, that it doesn’t work on all maps, and I don't know how to solve it, I will wait and see, how Vladislav is solving this problem.
If you want to test it, you could choose e.g. the new shirmish maps Obedska Bog or Sahyadri Buttes.

Stan added a comment.Apr 20 2020, 10:20 AM

The patch is waiting for reviewing, what update do you want to have? The patch is mostly working. As it have the same problem as the FXAA patch, that it doesn’t work on all maps, and I don't know how to solve it, I will wait and see, how Vladislav is solving this problem.
If you want to test it, you could choose e.g. the new shirmish maps Obedska Bog or Sahyadri Buttes.

@vladislavbelov is working on the issue. The problem lies in finding a good way to order shader passes because they cannot be used in any order (which could be overriden by maps)

OptimusShepard planned changes to this revision.Apr 27 2020, 10:05 AM

Waiting until D2712 is committed. A little modification of the code of this patch has to be done.

Stan added a comment.Apr 27 2020, 10:13 AM

Some comments for later. :)

binaries/data/mods/public/shaders/glsl/cas.fs
3 ↗(On Diff #11434)

2020? I guess it is still copyrighted?

source/renderer/PostprocManager.cpp
38 ↗(On Diff #11434)

Nuke.

598 ↗(On Diff #11434)

Wondering if it wouldn't be better to always call

m_SharpTech = g_Renderer.GetShaderManager().LoadEffect(CStrIntern(m_SharpName));

and let it error out if it isn't found.

Maybe even better if you can load the json to figure that out.

@OptimusShepard Please update it as soon as possible, I will help you test the latest CAS

OptimusShepard added inline comments.Apr 27 2020, 2:12 PM
binaries/data/mods/public/shaders/glsl/cas.fs
3 ↗(On Diff #11434)

Their Git seems to be long time not updated. It's still 2017-2019. How does it affect us?
(https://github.com/GPUOpen-Effects/FidelityFX)

Stan added inline comments.Apr 27 2020, 5:04 PM
binaries/data/mods/public/shaders/glsl/cas.fs
3 ↗(On Diff #11434)

I guess leave it as is. nvm :) From a quick google search I can't find anything.

Could you create the patch with context?

  1. Updating D2642: Contrast-Adaptiv-Sharpening pass #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Edit some comments.

Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Apr 27 2020, 11:23 PM

First time using Arcanist. Something went wrong, files are missing. How can I turn back to previous version?

Stan added a comment.Apr 28 2020, 12:00 AM

arc patch --diff 11434 --force

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

Linter detected issues:
Executing section Source...

source/renderer/PostprocManager.h
|  26| class·CPostprocManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPostprocManager{' 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/ps/CStrInternStatic.h
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2014"
Executing section JS...
Executing section cli...

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

Have you fixed it, my friend?

Revert to previous version and edit some comments.

OptimusShepard marked 2 inline comments as done.Apr 28 2020, 10:05 AM
OptimusShepard added inline comments.
source/renderer/PostprocManager.cpp
598 ↗(On Diff #11434)

You're right. If we check with the if (line 595) the allowed configurations, we don't need the errors, I think?

Stan added inline comments.Apr 28 2020, 10:18 AM
source/renderer/PostprocManager.cpp
598 ↗(On Diff #11434)

Well the question is what should we do when the user enters an incorrect / unsupported value, should we tell him?

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

Linter detected issues:
Executing section Source...

source/renderer/PostprocManager.h
|  26| class·CPostprocManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPostprocManager{' is invalid C code. Use --std or --language to configure the language.

source/ps/CStrInternStatic.h
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2014"

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.
Executing section JS...
Executing section cli...

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

OptimusShepard marked an inline comment as done.Apr 28 2020, 10:54 AM
OptimusShepard added inline comments.
source/renderer/PostprocManager.cpp
598 ↗(On Diff #11434)

Hm. As the user uses a drop down menu to select the filter, the only possibility to get an incorrect value is to edit the config file. But this wont lead to an crash. So is a error message really necessary? If we think so, we have to add error warnings to all parameter of the config file.

Stan added inline comments.Apr 28 2020, 10:56 AM
source/renderer/PostprocManager.cpp
598 ↗(On Diff #11434)

I guess keep it consistent with what is done currently then:)

Modified it, so it could be used on all maps.

Vulcan added a comment.May 3 2020, 1:41 PM

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

Linter detected issues:
Executing section Source...

source/renderer/PostprocManager.h
|  26| class·CPostprocManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPostprocManager{' 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/ps/CStrInternStatic.h
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2014"
Executing section JS...
Executing section cli...

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

Updating the licence and the link, as AMD had updated it. Changing the game options name to the AMD recommented one.

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/PostprocManager.h
|  26| class·CPostprocManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPostprocManager{' is invalid C code. Use --std or --language to configure the language.

source/ps/CStrInternStatic.h
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2014"
Executing section JS...
Executing section cli...

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

Shouldn't the CAS license be mentioned in LICENSE.txt?

Angen added inline comments.Jun 13 2020, 11:03 AM
source/ps/CStrInternStatic.h
1 ↗(On Diff #12062)

could you please update year?

Add license, change header year.

Shouldn't the CAS license be mentioned in LICENSE.txt?

I had looked on you fxaa patch. BSD is here correct, too?

OptimusShepard marked an inline comment as done.Jun 13 2020, 1:51 PM

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

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

Stan added a comment.Jun 13 2020, 2:23 PM

Tested on my UHD 630 and GTX1070 no problem. It even works nicely with the MSAA patch.

The patch looks good for me. Yes, it has duplications. But at the moment we can't make it more clear that really makes sense without a lot of code.

binaries/data/mods/public/gui/options/options.json
148 ↗(On Diff #12283)

I suppose sharpening should be also in dependencies.

binaries/data/mods/public/shaders/glsl/cas.fs
1 ↗(On Diff #12283)

We need to include #version here to understand for which GLSL version it was written.

Updated the versioning of the used GLSL version.

OptimusShepard marked an inline comment as done.Jul 22 2020, 11:14 PM
OptimusShepard added inline comments.
binaries/data/mods/public/gui/options/options.json
148 ↗(On Diff #12283)

Didn't find a possibility to use a element of the list as dependence. Using "sharpening" as dependence, sharpness is always locked. Sharpening isn't a boolean.

binaries/data/mods/public/gui/options/options.json
148 ↗(On Diff #12283)

Ok, I'll figure it later.

binaries/data/mods/public/gui/options/options.xml
12 ↗(On Diff #12866)

Does it look good for the minimal window size (1024x768)?

source/renderer/PostprocManager.h
98 ↗(On Diff #12866)

AFAIK m_Sharpness can be not initiliazed after CFG_GET_VAL("sharpness", m_Sharpness);, so I'd suggest to initialize in constructor.

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

Linter detected issues:
Executing section Source...

source/renderer/PostprocManager.h
|  26| class·CPostprocManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPostprocManager{' 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.
Executing section JS...
Executing section cli...

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

Edited the sharpness initialization. Disables sharpening, if post progressing, or GLSL, is disabled.

OptimusShepard marked 2 inline comments as done.Jul 25 2020, 12:06 AM
OptimusShepard added inline comments.
binaries/data/mods/public/gui/options/options.json
148 ↗(On Diff #12283)

Thx

binaries/data/mods/public/gui/options/options.xml
12 ↗(On Diff #12866)

I tested small window sizes like this. Seems to be ok.

OptimusShepard marked an inline comment as done.Jul 25 2020, 12:08 AM
OptimusShepard added inline comments.
source/renderer/PostprocManager.h
98 ↗(On Diff #12866)

I initialize it now with the default value.

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

Linter detected issues:
Executing section Source...

source/renderer/PostprocManager.h
|  26| class·CPostprocManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPostprocManager{' 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.
Executing section JS...
Executing section cli...

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

Stan added a comment.Aug 7 2020, 6:30 PM
In D2642#119996, @Stan wrote:

Tested on my UHD 630 and GTX1070 no problem. It even works nicely with the MSAA patch.

Still works fine :)

vladislavbelov added inline comments.Aug 7 2020, 9:55 PM
LICENSE.txt
33 ↗(On Diff #12895)

Isn't it MIT?

OptimusShepard added inline comments.Aug 7 2020, 11:30 PM
LICENSE.txt
33 ↗(On Diff #12895)

You're right. Thx

vladislavbelov accepted this revision.Aug 8 2020, 12:16 AM
This revision is now accepted and ready to land.Aug 8 2020, 12:16 AM