Page MenuHomeWildfire Games

Cleanup Camera and CGameView
ClosedPublic

Authored by vladislavbelov on Aug 20 2019, 1:29 AM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22755: Cleanup Camera and CGameView, removes a not needed method and refactors classes.
Summary

Removes not needed method and refactors classes.

Test Plan
  1. Apply the patch and compile the game
  2. Run tests and make sure that all are passed
  3. Run the game and atlas and make sure that everything works 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.Aug 20 2019, 1:29 AM

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

Linter detected issues:
Executing section Source...

source/graphics/Camera.h
|  41| class·CCamera
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCCamera{' is invalid C code. Use --std or --language to configure the language.

source/graphics/GameView.h
|  35| class·CGameView·:·private·Scene
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGameView:' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/View.cpp
| 211| »   »   //·not·in·every·call·to·g_Game->Update
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

Stan added a subscriber: Stan.Aug 20 2019, 10:39 AM

From CPPCheck, that you might want to fix.

Member variable 'CCamera::m_NearPlane' is not initialized in the constructor.
Member variable 'CCamera::m_FarPlane' is not initialized in the constructor.
Member variable 'CCamera::m_FOV' is not initialized in the constructor.
elexis added a subscriber: elexis.Aug 20 2019, 6:25 PM
elexis added inline comments.
source/graphics/Camera.cpp
49 ↗(On Diff #9408)

Why not use a pointer here? Just using a reference doesn't guarantee that it will exist for the entire function call.
And then add an if (camera) safeguard because it's a pointer and one doesnt want to null-deref.

vladislavbelov added inline comments.Aug 20 2019, 7:29 PM
source/graphics/Camera.cpp
49 ↗(On Diff #9408)

We don't store it, so it's not a problem since we're on the same stack. References aren't so simple as it may seem. For ex:

// int can be replaced by a class type.
int& foo() { int n = 42; return n; }
int& x = foo();
std::cout << x << std::endl; // <- that's not correct, it may crash or print a wrong value.

int foo() { int n = 42; return n; }
const int& x = foo();
std::cout << x << std::endl; // <- that's correct, as a constant reference extends a lifetime of its object.
// But that works only for local variables (not for an object members).
elexis added inline comments.Aug 20 2019, 7:48 PM
source/graphics/Camera.cpp
49 ↗(On Diff #9408)

So according to your advice you should use a pointer here:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

In C, if a function needs to modify a variable, the parameter must use a pointer

Stan added inline comments.Aug 20 2019, 7:55 PM
source/graphics/Camera.cpp
49 ↗(On Diff #9408)

We are not in C are we ?

elexis added inline comments.Aug 20 2019, 7:55 PM
source/graphics/Camera.cpp
49 ↗(On Diff #9408)

Sorry, wrong example since its const, but I hope you get the point regardless.

vladislavbelov added inline comments.Aug 20 2019, 8:03 PM
source/graphics/Camera.cpp
49 ↗(On Diff #9408)

In short my point about pointers/references:

  1. Pass const references for not modified objects.
  2. Use pointers for modified objects.
  3. Store links to objects as pointers (const for not modified).
elexis added inline comments.Aug 20 2019, 8:21 PM
source/graphics/Camera.cpp
145 ↗(On Diff #9408)

So this should use a pointer

164 ↗(On Diff #9408)

So this should use a pointer

189 ↗(On Diff #9408)

So this should use a pointer

source/graphics/GameView.cpp
386 ↗(On Diff #9408)

So this should return a pointer

409 ↗(On Diff #9408)

So this should return a pointer^

612 ↗(On Diff #9408)

So this should use a pointer

vladislavbelov added a comment.EditedAug 20 2019, 8:26 PM

@elexis In my opinion yes to mostly all your So this should *. I think it's more clear. But currently I don't see a strong reason to modify the code only for reference <> pointer transformation.

vladislavbelov added inline comments.Aug 20 2019, 8:27 PM
source/graphics/GameView.cpp
612 ↗(On Diff #9408)

I think it should be const CCamera& camera.

Stan added a comment.Aug 20 2019, 8:28 PM

@vladislavbelov will you fix the uninitialized members ?

@elexis In my opinion yes to mostly all your So this should *. I think it's more clear. But currently I don't see a strong reason to modify the code only for reference <> pointer transformation.

I don't necessarily agree that pointers are always preferable to non-const refs (I think for function arguments it's fine, I dislike reference member variables though), but I strongly agree that there is little reason to modify the code only for reference <> pointer transformation.

In D2195#91620, @Stan wrote:

@vladislavbelov will you fix the uninitialized members ?

Sure.

@elexis In my opinion yes to mostly all your So this should *. I think it's more clear. But currently I don't see a strong reason to modify the code only for reference <> pointer transformation.

So for T* it's obvious that it can be modified and for T& it is not obvious that it can be modified?
And for T* it is obvious that it cannot be null, because some comment in another file from X years ago says so?

In D2195#91641, @elexis wrote:

@elexis In my opinion yes to mostly all your So this should *. I think it's more clear. But currently I don't see a strong reason to modify the code only for reference <> pointer transformation.

So for T* it's obvious that it can be modified and for T& it is not obvious that it can be modified?

First - I don't understand why you're getting so worked up about this, nor going at this so disingenuously.

The point is more that usually, pointers aren't constant, and usually, references are constant. Doing something different is unexpected, and thus ever so slightly more dangerous. One is probably more likely to forget that a reference isn't constant than a pointer. Now as I've said, I am OK with non-const references in function arguments, since I agree their non-nullability is nice there...

And for T* it is obvious that it cannot be null, because some comment in another file from X years ago says so?

Nobody ever said that. A pointer is obviously nullable. Checking for nullability everywhere doesn't make sense, though - and switching to a reference just says "don't check here". Cause you can't. But it does nothing to actually guarantee that the lifetime is being extended if some other code is doing something stupid. So the bug can still be there.

Fixes compiler warnings from @Stan.

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

Linter detected issues:
Executing section Source...

source/graphics/Camera.h
|  41| class·CCamera
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCCamera{' is invalid C code. Use --std or --language to configure the language.

source/graphics/GameView.h
|  35| class·CGameView·:·private·Scene
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGameView:' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/View.cpp
| 211| »   »   //·not·in·every·call·to·g_Game->Update
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

In D2195#91641, @elexis wrote:

@elexis In my opinion yes to mostly all your So this should *. I think it's more clear. But currently I don't see a strong reason to modify the code only for reference <> pointer transformation.

So for T* it's obvious that it can be modified and for T& it is not obvious that it can be modified?

First - I don't understand why you're getting so worked up about this

Because you have wasted several hours of my lifetime, telling me that my patch is bad because it changes code without you understanding the point of it, accusing me of not understanding references, accusing me of being disingenous, making strawmans and proposing to change the coding conventions to use pointers when it can use references.

nor going at this so disingenuously.

I find this accusation quite worrying and I will remember it.

The point is more that usually, pointers aren't constant, and usually, references are constant.

If you remove the word "usually" the sentence is completely false.
So what does "usually" mean? The Google code that Vladislav linked? If they have one consistent codebase I would add code consistently. Or our codebase? Or the ideal codebase to which we should move?
Vladislav is actually arguing to prefer pointers when references are just as valid, and this has objective disadvantages.

Doing something different is unexpected, and thus ever so slightly more dangerous. One is probably more likely to forget that a reference isn't constant than a pointer.

So T* appears changeable, T& appears constant, no I don't follow. One could also confuse a pointer or a reference with a copy.
But one thing that cannot happen is that one confuses a pointer that can be null with a pointer that cannot be null if one uses a reference.

And for T* it is obvious that it cannot be null, because some comment in another file from X years ago says so?

Nobody ever said that. A pointer is obviously nullable. Checking for nullability everywhere doesn't make sense, though - and switching to a reference just says "don't check here". Cause you can't

Yes a pointer can assume null. A reference cannot. That is the point of prefering a reference if non-null is given.
The reason to prefer a reference is that you don't have to look at a function comment or at the callstack to determine whether you have to check for null or not.

But it does nothing to actually guarantee that the lifetime is being extended if some other code is doing something stupid. So the bug can still be there.

Nor did I ever claim that using a reference changes the lifetime of the referred value for the fifth time in the last 24 hours.

Patch looks cool.

Because you have wasted several hours of my lifetime, telling me that my patch is bad because it changes code without you understanding the point of it

I think I've perfectly understood your patch and I still think it's bad (or rather, not worth committing). I have to guess at this point that you believe this to be so unthinkable that you must assume I've misunderstood your patch to reconcile that with reality somehow?
You still haven't acknowledged this remark:

But currently I don't see a strong reason to modify the code only for reference <> pointer transformation.

I would have accepted "well, I believe the gain here of explicitly not needing to check for nulls is worth it." I would disagree, but you know that's a thing that happens in life, and it wouldn't be a big deal.
For some reason which I can't fathom you made a whole pataquès out of this, as if we were debating some universal law of something. And then when both I and Vlad said we disagreed with your patch (perhaps for different reasons), you quoted Gandhi and said you were right anyways and didn't care for our opinon. It's a bit hard to take you seriously.

proposing to change the coding conventions to use pointers when it can use references.

-_-

I find this accusation quite worrying and I will remember it.

¯\_(ツ)_/¯

The point is more that usually, pointers aren't constant, and usually, references are constant.

If you remove the word "usually" the sentence is completely false.

Why even write that? Yes, if you change words, sentences change meaning? This is exactly what I mean with "disingenusouly" above.

So what does "usually" mean?

Well if you do want some more details, I would ask you to fetch all c++ LOC from GitHub, check how many times a reference is const vs non-const, and do the same for pointers. I have no doubt that my "usually" above would stand.

So T* appears changeable, T& appears constant, no I don't follow.

Well that's where you should stop discussing this, because we obviously speak from different understandings of the universe, and they seem completely impossible to reconcile.
As for me, I enjoy trolling too much to stop just now.

The reason to prefer a reference is that you don't have to look at a function comment or at the callstack to determine whether you have to check for null or not.

And (as I've said several times too if you want to play this game), I think this 'pro' of references is not worth the cost of having a non-const member-reference in IGuiObject. But again, you've not addressed that comment, I guess you don't see the point of it.

@wraitii any thoughts about updated patch? Is it acceptable?

wraitii accepted this revision.Aug 22 2019, 8:22 AM

@wraitii any thoughts about updated patch? Is it acceptable?

Sorry for the OT, I think the patch is good.

This revision is now accepted and ready to land.Aug 22 2019, 8:22 AM