Page MenuHomeWildfire Games

Refactors Camera aspect ratio and calculation of plane points
ClosedPublic

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

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22547: Refactors Camera aspect ratio and calculation of plane points.
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
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.

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.Jul 4 2019, 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.Jul 14 2019, 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 ↗(On Diff #8865)

(perhaps getViewPortAspectRatio, I dont mind)

137 ↗(On Diff #8865)

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 ↗(On Diff #8865)

(Formally I wonder if the loop performance differs)

172 ↗(On Diff #8865)

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

source/graphics/Camera.h
28 ↗(On Diff #8865)

tanf in <math.h>

This revision is now accepted and ready to land.Jul 14 2019, 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

This revision was landed with ongoing or failed builds.Jul 25 2019, 1:08 AM
This revision was automatically updated to reflect the committed changes.
elexis added a comment.EditedAug 8 2019, 3:10 PM

Fixes @elexis's notes from IRC (2019-07-03#0ad-dev):
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.

It would construct one Vector3D and then assign it, probably copy-assign too. So indeed.
The redundant quad[i] references should be most likely optimized.

But perhaps we could add a Vector3D Set method that consumes the 3 arguments, making it more failsafe (as coordinates can only be assigned in triplets, making it less of a chance to assign to a wrong index etc.).