Page MenuHomeWildfire Games

Cleanup of SkyManager
ClosedPublic

Authored by vladislavbelov on Nov 9 2018, 10:10 PM.

Details

Reviewers
Imarok
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP21952: Cleanup and simple refactor the SkyManager.
Summary

Cleanup and simple refactoring of SkyManager. It makes more clear a way to improve SkyManager.

Test Plan
  1. Apply & compile the patch
  2. Run the game and check that the sky still works, and that it's toggleable.

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.Nov 9 2018, 10:10 PM
Vulcan added a subscriber: Vulcan.Nov 9 2018, 10:17 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/ActorViewer.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/ActorViewer.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/ActorViewer.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...

Link to build: https://jenkins.wildfiregames.com/job/differential/777/

Stan added a subscriber: Stan.Nov 9 2018, 11:31 PM

Would be nice to use cacheloader to load PNG as well

Imarok accepted this revision.Nov 10 2018, 9:41 PM
Imarok added a subscriber: Imarok.

Else looks good

source/renderer/SkyManager.cpp
151–152 ↗(On Diff #6979)

Maybe you also want to get rid of those. They contradict our current style.

source/renderer/SkyManager.h
64–72 ↗(On Diff #6979)

Why are those needed?
Seems like useless Getters and Setters...

This revision is now accepted and ready to land.Nov 10 2018, 9:41 PM
vladislavbelov marked an inline comment as done.Nov 12 2018, 5:09 PM
vladislavbelov added inline comments.
source/renderer/SkyManager.h
64–72 ↗(On Diff #6979)

They are used in Atlas below.

Imarok added inline comments.Nov 12 2018, 7:45 PM
source/renderer/SkyManager.h
64–72 ↗(On Diff #6979)

Sure, but why do we need a Getter and a Setter instead of raw access?
Usually Getters and Setters are used to restrict access or do some side-effect while access. Here that is just unneeded.

vladislavbelov marked an inline comment as done.Nov 12 2018, 8:54 PM
vladislavbelov added inline comments.
source/renderer/SkyManager.h
64–72 ↗(On Diff #6979)

Because C++ doesn't have any simple access monitoring (because low-level). So we don't allow to access class members (especially private) to modify without notifying the parent class (by notifying means the methods calling).

So it's the good practice to know when someone modifies your class state.

Imarok added inline comments.Nov 17 2018, 8:36 PM
source/renderer/SkyManager.h
64–72 ↗(On Diff #6979)

But why do we want to know it here?
(I fully understand the usecase of getters and setters in general, but here I just don't see the benefit)

vladislavbelov marked an inline comment as done.Nov 18 2018, 1:08 AM
vladislavbelov added inline comments.
source/renderer/SkyManager.h
64–72 ↗(On Diff #6979)

We use the principe: class members aren't public, only struct ones are. We use classes like a black box, we don't want to show people how they work inside. We share only interfaces.

Besides it, we'd need to do some staff here on the state changes, because I wanna add a more complex sky rendering technique.

Imarok added inline comments.Nov 18 2018, 10:04 AM
source/renderer/SkyManager.h
64–72 ↗(On Diff #6979)

Then it's totally ok.

This revision was automatically updated to reflect the committed changes.