Uses correct values of clip planes instead of config ones (as camera settings might differ).
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.
- 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
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
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 :)
I can't say that it's a bug fix or a cleanup. What's about "Uses correct clip planes values in PostProcManager"?
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. |
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 ? |
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 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.)? |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/440/display/redirect
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) |
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.
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.) |