- Apply the patch and compile the game
- Run tests and make sure that they're passed
- Run the game and make sure that clicking on units/minimap camera bounds work as before
Details
- Reviewers
elexis - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22547: Refactors Camera aspect ratio and calculation of plane points.
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.
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
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>; ?
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?
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.
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
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
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. |
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> |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/153/display/redirect
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.).