Page MenuHomeWildfire Games

Pass CVector3D as const reference in a few places.
ClosedPublic

Authored by leper on May 4 2017, 4:18 AM.

Details

Reviewers
Sandarac
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP19534: Pass CVector3D as const reference in a few places.
Summary

Make these few places do what the rest of the codebase does.

Test Plan

Compile, check that it does the same.

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

leper created this revision.May 4 2017, 4:18 AM
Owners added a subscriber: Restricted Owners Package.May 4 2017, 4:18 AM

Nice, I noticed those missing const refs in the SoundManager code when working on P41.

source/graphics/Camera.cpp
362 ↗(On Diff #1636)

I don't know much about this part of the code, but it seems this change causes cinema cut scenes to fail (tested on the "Cinema Demo" map).

leper planned changes to this revision.May 4 2017, 5:01 AM
leper added inline comments.
source/graphics/Camera.cpp
364 ↗(On Diff #1636)

The use of orientation here is what causes the breakage (since this one isn't normalized, but should be). Nice catch!

leper added inline comments.May 4 2017, 5:15 AM
source/graphics/Camera.cpp
364 ↗(On Diff #1636)

Actually the use of up is broken too...

vladislavbelov added inline comments.
source/graphics/Camera.cpp
364 ↗(On Diff #1636)

Not only, orientation should be normalized too (it doesn't without normalization, I've tested). Also it'd be not bad to normalize s too, I think.

Vulcan added a subscriber: Vulcan.May 4 2017, 6:51 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/997/ for more details.

leper updated this revision to Diff 1751.May 8 2017, 5:01 AM

Keep two parameters as copies, since otherwise we'd just have to create a copy right afterwards.

leper marked 4 inline comments as done.May 8 2017, 5:03 AM
leper added inline comments.
source/graphics/Camera.cpp
364 ↗(On Diff #1636)

Actually keeping orientation and up as pass-by-value, since otherwise we'd just have to create a copy right afterwards.

Sandarac accepted this revision.May 8 2017, 5:43 AM

Well, this diff doesn't seem to break cinematic camera functionality anymore :)

This revision is now accepted and ready to land.May 8 2017, 5:43 AM
This revision was automatically updated to reflect the committed changes.
leper marked an inline comment as done.
Vulcan added a comment.May 8 2017, 5:50 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1079/
See console output for more information: http://jw:8080/job/phabricator/1079/console