Page MenuHomeWildfire Games

Refactors Camera aspect ratio and calculation of plane points
AcceptedPublic

Authored by vladislavbelov on Fri, Jun 28, 1:42 AM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Test Plan
  1. Apply the patch and compile the game
  2. Run tests and make sure that they're passed
  3. Run the game and make sure that clicking on units/minimap camera bounds work as before

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 8330
Build 13601: Vulcan BuildJenkins

Event Timeline

vladislavbelov created this revision.

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

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

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

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

Fixes styles.

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

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

Suggestion: wouldn't it be better to rename "Get[Near/Far]Plane" to "Get[Near/Far]PlaneDistance", and name GetCameraPlanePoints something like GetPlaneCorners (Camera is a given since we're in CCamera).

Also, this comes down to personal preference I guess, but I think CVector3D points[4] is (far) more readable than std::array<CVector3D, 4> points. Maybe with a using PlaneCorners = std::array<CVector3D, 4>; ?

Suggestion: wouldn't it be better to rename "Get[Near/Far]Plane" to "Get[Near/Far]PlaneDistance"

I don't mind, only note that the distance in the camera space.

name GetCameraPlanePoints something like GetPlaneCorners (Camera is a given since we're in CCamera).

Actually plane can't have corners, it's infinity. We might only have points on it. Or we could use quad.

Also, this comes down to personal preference I guess, but I think CVector3D points[4] is (far) more readable

Probably, but it's (far) more unsafe, as you can pass any pointer (even a pointer on a single element). Also you can't use range-based loop for them.

Maybe with a using PlaneCorners = std::array<CVector3D, 4>; ?

Maybe using Quad = std::array<CVector3D, 4>; then?

I don't mind, only note that the distance in the camera space.

Sure. I mean near/far plane will be obvious to anybody who has worked on this, so I guess it might be fine as is, particularly with the below.

Maybe using Quad = std::array<CVector3D, 4>; then?

Sounds good to me.

Adds CCamera::Quad.

vladislavbelov planned changes to this revision.Thu, Jul 4, 12:05 AM

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

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

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

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

vladislavbelov added a subscriber: elexis.

Fixes @elexis's notes from IRC (2019-07-03#0ad-dev):

22:03 < elexis> ++i
22:03 < elexis> ranged loop
22:04 < elexis> (float)px / (float)g_Renderer.GetWidth() -> static cast

I didn't fix 22:02 < elexis> Vladislav: quad[0] = { ... } ?, because I didn't check that the code have to be inlined. So probably later.

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

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

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

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

elexis accepted this revision.Sun, Jul 14, 12:13 AM

Compared the previous code against the new one and found no behavior changes.
Tested in accordance with the phrased testplan, didn't explode.
Didn't check the test, but it succeeded.

I didn't fix 22:02 < elexis> Vladislav: quad[0] = { ... } ?, because I didn't check that the code have to be inlined. So probably later.

Too bad for readability, but better don't taske risks.

Feel free to perform renames or changes mentioned in the inline comments.

source/graphics/Camera.cpp
135

(perhaps getViewPortAspectRatio, I dont mind)

137

Since this function is called on Render, perhaps the GetAspectRatio should be cached.
(Perhaps one can think about an tanf approximation some time too, though it probably negligible here.)

166

(Formally I wonder if the loop performance differs)

172

(why are the floats made const but not the vectors)

source/graphics/Camera.h
28

tanf in <math.h>

This revision is now accepted and ready to land.Sun, Jul 14, 12:13 AM

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

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