Splits Camera control from CGameView to separate file
ClosedPublic
Actions

Authored by vladislavbelov on Oct 2 2019, 1:55 AM.

Details

Reviewers

None

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP23031: Splits Camera control from CGameView to separate file.

Summary

Currently CGameView contains view/rendering logic and camera control logic.

Currently CameraController contains some places (for ex. header members) that can be optimized. But I want to minimize number of changes for movement.

Test Plan

Apply the patch and compile the game
Run the game and make sure that everything works
Run tests and make sure that all tests are passed 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

vladislavbelov created this revision.Oct 2 2019, 1:55 AM

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

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

Harbormaster failed remote builds in B9689: Diff 10045!Oct 2 2019, 1:55 AM

vladislavbelov retitled this revision from Splits Camera control to separate file to Splits Camera control from CGameView to separate file.Oct 2 2019, 1:56 AM

vladislavbelov edited the summary of this revision. (Show Details)

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/378/display/redirect

Nothing objectionable discovered from my side in the brief review without testing.
When moving code around, one should check that one splits into helper functions or classes where benefiital (taking the opportunity to chose the cleaner alternative if already moving those lines).
One could check that memory layout and performance aren'tt worsened if possible.
One could also generate an inlining report with clang if one is curious about that, but it probably doesn't say much.

source/graphics/CameraController.cpp
105 ↗	(On Diff #10045)	Rename or added subroutine `CCameraController::LoadConfig()`? New code that would be inserted into some Init function is logically separate from this code.
140 ↗	(On Diff #10045)	(Could be a C++ function, but would perform string concatenation unless one specifies the full config value path)
170 ↗	(On Diff #10045)	(static locals are a hack usually)
245 ↗	(On Diff #10045)	This and three scopes below look like they might be moved to a helper function. Dunno about performance impact (but there might also be the code extensibility consideration).
270 ↗	(On Diff #10045)	unneeded else after return, but doesnt look bad to have the two scopes. There might be a third scope sometime.
402 ↗	(On Diff #10045)	comment from rP7930 Commented out code is irritating. If its valuable code, why isnt it code so that it can be maintained (perhaps behind a condition)). If it is not valuable code, why is it near the valuable code? So when is the time when this becomes necessary? Can the user provide some bad config values and have it become necessary? If so, the C++ code could sanitize the values too and then still remove this code.(?) If its something a mod or new game reusing pyrogenesis could trigger, then it sounds like this code should be run.
source/graphics/CameraController.h
130 ↗	(On Diff #10045)	\n

Stan added a subscriber: Stan.Oct 2 2019, 11:17 AM

Stan added inline comments.

source/graphics/CameraController.cpp
402 ↗	(On Diff #10045)	I guess this code would allow one to stop going through the map in Atlas. So since you can reset the camera anyway and it's useful to see decaying corpses, I guess it could be removed alltogether. (Assuming the comment is accurate and the code does what it says)

In D2347#98146, @elexis wrote:

When moving code around, one should check that one splits into helper functions or classes where benefiital (taking the opportunity to chose the cleaner alternative if already moving those lines).

I agree that the code should be refactored, but at the same time I want to keep the diff as small as possible (only movement).

One could check that memory layout and performance aren'tt worsened if possible.
One could also generate an inlining report with clang if one is curious about that, but it probably doesn't say much.

The controller is a member, so performance shouldn't be a problem.

Stan added inline comments.Oct 2 2019, 2:21 PM