Paths

Table of Contentst

Differential D2196

Fixes values of clip planes in PostProcManager
ClosedPublic
Actions

Authored by vladislavbelov on Aug 20 2019, 2:02 AM.

Details

Reviewers

wraitii

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP22940: Fixes values of clip planes in PostProcManager.
rP22738: Moves and cleanups includes and public/private section of PostprocManager.

Summary

Uses correct values of clip planes instead of config ones (as camera settings might differ).

Test Plan

Apply the patch and compile the game
Run tests and make sure that all are passed
Run the game and atlas and make sure that everything works as before

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.Aug 20 2019, 2:02 AM

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

Linter detected issues:
Executing section Source...

source/renderer/PostprocManager.h
|  24| class·CPostprocManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPostprocManager{' 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/432/display/redirect

Harbormaster completed remote builds in B8932: Diff 9409.Aug 20 2019, 2:09 AM

Stan added a subscriber: Stan.Aug 20 2019, 7:29 AM

Stan added inline comments.

source/renderer/PostprocManager.cpp
604 ↗	(On Diff #9409)	Missing UNUSED ?

Looks alright, but calling it a 'refactoring' is a bit misleading imo, I'd avoid it in the commit message :)

In D2196#91405, @wraitii wrote:

Looks alright, but calling it a 'refactoring' is a bit misleading imo, I'd avoid it in the commit message :)

I can't say that it's a bug fix or a cleanup. What's about "Uses correct clip planes values in PostProcManager"?

elexis added a subscriber: elexis.Aug 20 2019, 2:13 PM

elexis added inline comments.

source/renderer/PostprocManager.cpp
32 ↗	(On Diff #9409)	(The reordering could be in the commit that doesnt change code behavior too)
source/renderer/PostprocManager.h
115 ↗	(On Diff #9409)	I'd recommend to split the moving of lines of code that doesnt change behavior from the part that does, so that it's easier to audit. You can consider this hunk and the yellow lines above reviewed if you want.

Stan added inline comments.Aug 20 2019, 2:20 PM

source/renderer/PostprocManager.h
41 ↗	(On Diff #9409)	@elexis according to your previous message on D2197#inline-42980 shouldn't that inline be removed too ?

In D2196#91489, @vladislavbelov wrote:

In D2196#91405, @wraitii wrote:

Looks alright, but calling it a 'refactoring' is a bit misleading imo, I'd avoid it in the commit message :)

I can't say that it's a bug fix or a cleanup.

What's about "Uses correct clip planes values in PostProcManager"?

This suggests that it used incorrect clip planes previously, i.e. implying it to be a bugfix.

If it's just a new setter function, then it's more of a feature, something like "Support doing foo foo"...

source/renderer/PostprocManager.cpp
604 ↗	(On Diff #9409)	Do we know the function to differ for both cases or could one define it outside of the macro and assume to be equal? I don't mind
source/renderer/Renderer.cpp
1351 ↗	(On Diff #9409)	-\n i.e. `GetFarPlane());` Also notice that this increases the number of function calls, but perhaps just setting the two members directly reduces the stack by 1-3 function calls (so as to optimize even with non-optimized compilers). (It's called each RendererSubmit, so not outlandish to consider.)?

vladislavbelov added inline comments.Aug 20 2019, 3:29 PM

source/renderer/PostprocManager.h
115 ↗	(On Diff #9409)	Can I commit the movement separately without making a diff?
source/renderer/Renderer.cpp
1351 ↗	(On Diff #9409)	`-\n` seems ok to me, probably we need to pin in CC. It costs mostly nothing and it's called only once per frame.

elexis added inline comments.Aug 20 2019, 3:32 PM

source/renderer/PostprocManager.h
115 ↗	(On Diff #9409)	That's what I meant with the last sentence.
source/renderer/Renderer.cpp
1351 ↗	(On Diff #9409)	What value is it adding if one could also get away with less cycles? (Not that I object, but if it seems avoidable why not avoid it)

elexis added a commit: rP22738: Moves and cleanups includes and public/private section of PostprocManager..Aug 20 2019, 11:25 PM

Committed cleanups separately in rP22738.

vladislavbelov retitled this revision from Refactoring of PostProcManager to Fixes values of clip planes in PostProcManager.Aug 20 2019, 11:38 PM

vladislavbelov edited the summary of this revision. (Show Details)

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

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

Harbormaster failed remote builds in B8981: Diff 9425!Aug 21 2019, 12:14 AM

Patch looks a lot cleaner already, doesn't it? The committed one as well.

So the idea is that it should use m_ViewCamera (from rP3405) instead of g_Game->GetView().
It's currently always identical but if someone was to come along and change the camera projection, it would read the value from the wrong place?

I wonder if one could even avoid the call each frame by calling SetDepthBufferClipPlanes when m_ViewCamera changes?
(Not only for performance, but might also be cleaner code-wise to set a value that depends on another value only if and only if that other value changes)
That would be SetViewCamera, because those m_ViewCamera = normalCamera assignments are just temporary back and forth?

Otherwise I won't object to you committing this if you are certain that you know what you are doing (and I can't complain about a commit if there is no reason to complain in the commit).

source/renderer/Renderer.cpp
1351 ↗	(On Diff #9409)	(-\n if you don't mind)

In D2196#91706, @elexis wrote:

So the idea is that it should use m_ViewCamera (from rP3405) instead of g_Game->GetView().

Not quite right, m_ViewCamera is the current camera of Renderer (which can be changed during frame rendering, currently for shadow maps), but here we need exact values used for the final frame before postprocessing.

I wonder if one could even avoid the call each frame by calling SetDepthBufferClipPlanes when m_ViewCamera changes?

Nope, m_ViewCamera isn't what we exactly need.

Change is positive and this works on my machine -> Accept.

This revision is now accepted and ready to land.Sep 18 2019, 9:02 PM

This revision was landed with ongoing or failed builds.Sep 20 2019, 9:54 AM

Closed by commit rP22940: Fixes values of clip planes in PostProcManager. (authored by vladislavbelov). · Explain Why

This revision was automatically updated to reflect the committed changes.

vladislavbelov added a commit: rP22940: Fixes values of clip planes in PostProcManager..

elexis added inline comments.Sep 20 2019, 1:15 PM

source/renderer/Renderer.cpp
1351 ↗	(On Diff #9409)	(For scopes it makes sense to put it on a separate line, for JS arrays and JS Objects too, but I didn't see the reason why the \n should be on a separate line rather than after the last argument for function calls. It leaves the impression of a scope or object being created. One can also count the occurrences of the two styles. I used this \n style for few commits too, but I quickly reverted my opinion.)