Page MenuHomeWildfire Games

Cleanup Camera code for projections
ClosedPublic

Authored by vladislavbelov on May 22 2018, 12:18 AM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22033: Cleanup Camera code for projections.
Summary

The patch splits the camera and projections, makes the SetProjectionTile function independent from previous calls.

Test Plan
  1. Apply the patch and compile the game
  2. Run the game with any map
  3. Take a big screenshot
  4. Make sure, that it looks the same as before the patch

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

Vulcan added a subscriber: Vulcan.May 22 2018, 12:37 AM

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

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

Stan added a subscriber: Stan.Jun 3 2018, 11:38 PM
Stan added inline comments.
source/graphics/Camera.cpp
67 ↗(On Diff #6612)

Doxygen type comment ?

source/graphics/Camera.h
71 ↗(On Diff #6612)

Returns ?

vladislavbelov added inline comments.Jun 4 2018, 12:12 AM
source/graphics/Camera.cpp
67 ↗(On Diff #6612)

Doxygen comments should be in headers.

source/graphics/Camera.h
71 ↗(On Diff #6612)

True.

Stan added inline comments.Jun 4 2018, 12:48 AM
source/graphics/Camera.cpp
67 ↗(On Diff #6612)

Well functions have doxygen comments in theory.

vladislavbelov added inline comments.Jun 4 2018, 1:34 AM
source/graphics/Camera.cpp
67 ↗(On Diff #6612)

But Doxygen comments usually describes an API, not an implementation behind this (but not always).

Fixes the note.

vladislavbelov marked an inline comment as done.Jun 4 2018, 1:56 AM
Stan added a comment.Dec 28 2018, 3:40 PM

Couldn't do big screenshots because the game wouldn't let me (OOM) but this standard screenshots are the same, without or without the patch. Both looks the same. Looks good to me.

source/graphics/Camera.cpp
47 ↗(On Diff #6718)

Didn't know that notation existed, but in this case, do you have to define a destructor at all ?

source/maths/Matrix3D.cpp
78 ↗(On Diff #6718)

I do not know how intensive this multiplication is but you might want to store that in a value and use it for both.

vladislavbelov marked 2 inline comments as done.Dec 28 2018, 3:46 PM
vladislavbelov added inline comments.
source/graphics/Camera.cpp
47 ↗(On Diff #6718)

It's not required here, but it may save some compilation time. Because you don't need to change the header to change the destructor behaviour.

source/maths/Matrix3D.cpp
78 ↗(On Diff #6718)

I don't think that it makes sense here. Because it's the simple expression that can be easily optimised by a compiler.

If try to optimise, then it'd be written like:

	const float tiles_f = 1.f / tanf(fov / 2.f) * tiles;

	SetPerspective(fov, aspect, near, far);
	_11 = tiles_f / aspect;
	_22 = tiles_f;
	_13 = -(1 - tiles + 2 * tile_x);
	_23 = -(1 - tiles + 2 * tile_y);
wraitii accepted this revision.Jan 5 2019, 10:51 AM
wraitii added inline comments.
source/maths/Matrix3D.cpp
78 ↗(On Diff #6718)

definitely optimised by the compiler.

This revision is now accepted and ready to land.Jan 5 2019, 10:51 AM
This revision was automatically updated to reflect the committed changes.