Page MenuHomeWildfire Games

Refactors Camera projections
ClosedPublic

Authored by vladislavbelov on Wed, Jun 26, 1:04 AM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22404: Refactors camera projections - makes projection functions of camera more clear.
Summary

We define the Camera as a single object that contains only orientation (position + rotation) and projection at some moment. We use this information to render a scene and additional things, like a shadow map. So the Camera doesn't contain any animation, event handling and so on, purely graphics object.

Test Plan
  1. Apply the patch and compile the game with tests
  2. Make sure that all tests are passed
  3. Make sure that everything looks 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.Wed, Jun 26, 1:04 AM
vladislavbelov added inline comments.Wed, Jun 26, 1:07 AM
source/graphics/Camera.h
49 ↗(On Diff #8612)

I didn't use SetPerspectiveProjection, because I think it's too long and it doesn't help much (because perspective camera is a known word collocation).

109 ↗(On Diff #8612)

I didn't move it under private too, because there're many occurrences of its using and this patch is only about projections.

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

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/View.cpp
| 211| »   »   g_Game->Interpolate(actualFrameLength,·realFrameLength);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

elexis added a subscriber: elexis.Wed, Jun 26, 1:25 AM
elexis added inline comments.
source/graphics/Camera.h
49 ↗(On Diff #8612)

(One could add comments for the functions)

I wouldn't disagree with SetPerspectiveProjection, even if it's a bit longer, so that it's clear from the name that this in the end sets the projection, so that the reader sees both Set*Projection functions have this in common.

SetProjection, SetPerspective sound more like two functions changing something different.

(I'm speaking on the naming because the naming is the only thing changed in this diff)

I won't object if you prefer SetPerspective.

Renames SetPerspective to SetPerspectiveProjection.

vladislavbelov added inline comments.Wed, Jun 26, 7:15 PM
source/graphics/Camera.h
49 ↗(On Diff #8612)

Probably SetPerspectiveProjection is more clear at the moment.

I'm not sure about comments, functions do what their names tell.

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

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/View.cpp
| 211| »   »   g_Game->Interpolate(actualFrameLength,·realFrameLength);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

elexis accepted this revision.Wed, Jun 26, 7:33 PM

The rename is acceptable for me. Not compiled, except for seeing Jenkins suceed and this not doing more than a rename and one private-flagging.

A quick search for m_ProjMat

graphics/Camera.cpp:	m_ProjMat.SetPerspective(m_FOV, aspect, m_NearPlane, m_FarPlane);
graphics/Camera.cpp:	m_ProjMat.SetPerspectiveTile(m_FOV, aspect, m_NearPlane, m_FarPlane, tiles, tile_x, tile_y);
graphics/Camera.cpp:	MatFinal = m_ProjMat * MatView;
graphics/Camera.cpp:	CMatrix3D transform = m_ProjMat * m_Orientation.GetInverse();
renderer/Renderer.cpp:	camera.m_ProjMat = scaleMat * camera.m_ProjMat;
renderer/Renderer.cpp:	camera.m_ProjMat = scaleMat * camera.m_ProjMat;

shows that it seems that the change is complete too.

source/tools/atlas/GameInterface/View.cpp
225 ↗(On Diff #8621)

(I guess most compilers are smart enough to inline)

This revision is now accepted and ready to land.Wed, Jun 26, 7:33 PM
Stan added a subscriber: Stan.EditedWed, Jun 26, 8:00 PM

Tested it on windows. Noticed no apparent glitches, nor errors, rotated camera zoomed in and out, and did both at the same time. Parallax look the same.

EDIT: Sorry didn't run the tests.

Stan added a comment.Wed, Jun 26, 8:11 PM

Test passed on windows :)

This revision was automatically updated to reflect the committed changes.