Encapsulation is obvious when I have some basic class with a few ints, floats, etc (return by copy). But often I have to either :

1- deal with bigger objects where I can't afford copying, or copying wouldn't make sense

2- polymorphic object which has to be a pointer / reference, so it can't be returned by value.

For example,

class Scene

{

std::vector<Entity*> m_Entities;

//...

//Some other big data

public:

//some member functions

void InitSkyBox(const SkyboxInfo&);

};

class Engine

{

std::unique_ptr<Scene> m_CurrentScene;

public:

void LoadScene(const std::string& sceneStr);

void UnloadScene();

Scene* GetScene() const {return m_CurrentScene.get();}

};

void clientCode(...)

{

Scene* curScene = engine->GetScene();

curScene->InitSkyBox(skyboxinfo);

//...

}

Engine is responsible for the life time of the scene, as it manage loading / unloading etc. Many objects may need to access the scene. For example if I have an object that controls the skyboxes, I could need to get the scene, retrieve the current skybox and modify it. I could also need the scene object to raycast and query a list of entities.

What would be the best approach for this ? If I understand correctly Encapsulation is broken since I returned the scene ptr (handle to internal data) and the client code can do whatever it wants without Engine being aware.

My options would be :

Option A : "Copy" every member function of scene in engine and use delegation instead :

void Engine::InitSkyBox(const SkyboxInfo& info)

{

if(m_Scene.get())

{

m_Scene->InitSkybox(info);

}

}

Now Engine has better control over the scene, but that wouldn't scale well, it could duplicate every methods of scene.

Option B : Return a const pointer instead.

const Scene* const GetScene() const {return m_Scene.get();}

Seems a bit better, but I'm still returning an "handle" to an internal (see Scott Meyers effective c++), and can't call a non const operation.

So, if Scene is a well encapsulated object, how bad is it to return it from the class that is responsible for its lifetime ? I know that objects that refer to the scene could have dangling pointers if the Engine dies before it, but that would be unlikely.

In a game engine, unfortunately, you would also need to retrieve entities / components from everywhere, breaking your scene's encapsulation of entities.

I'm curious to see how you deal with this in your code. Not sure if there is a silver bullet for this :)

Thanks,

X

If I have a std::vector of std::string, and std::string is a well-encapsulated object, is it bad that std::vector gives me access to the std::strings, since std::vector manages the lifetime of those strings for me?

std::vector<std::string> strArray;
strArray.back() = "blah"; //Modifies the string that the std::vector manages for me.
stdArray[5] = "blah"; //More modifications.
stdArray.at(5) = "blah"; //And more.

However, if Engine only ever has one scene at a time (since you have it in a unique_ptr), why not make it a non-pointer member variable?
Also, unless Engine needs to do special things when the scene is modified (unlikely, since you access the pointer from outside of Engine), why not have it public?

class Engine
{
	public:
	Scene scene; //Not evil. You won't get arrested for doing this.
};

It doesn't need to be named 'current' if it's the only one.
It doesn't need to be private if you access it from outside of Engine anyway, and if outside modifications don't upset Engine.
It doesn't need to be a pointer unless you need it dynamically allocated, and if you do need it dynamically allocated, those allocations should probably happen inside of Scene anyway, not by classes using Scene.

Do you do this in your code?

std::unique_ptr<std::string> myString;

std::string manages its memory itself (aside from small-string optimizations), you don't need to have all your strings be wrapped in smart pointers. Sometimes you do, for memory or performance reasons, but in the general case, let std::string manage itself. In the general case, 'Scene' should manage itself.

If Scene contains a std::vector... std::vector manages its memory dynamically. It doesn't cost the owning class much to have: std::vector<HeavyData>.

In the general sense, that's what? Two ints and a pointer? (element count, capacity count, and allocated memory pointer). Oh, and the allocator, a few more bytes.

Are you copying 'Engine' alot? If not, why do you care if Engine has a non-pointer Scene? If Engine only has one Scene at a time anyway, it might as well be allocated in the same block of memory as Engine is (unless you need to control the exact construction/destruction time of Scene separately from the construction/destruction of Engine).

Having Scene private inside Engine is an illusion if you pass around non-const pointers to it - might as well make it public!

Unless, you actually need to have Engine adjust, constrain, or otherwise be aware of changes to Scene, in which case you can't help but keep Scene private and replicate most of Scene's interface in Engine and keep Scene only const-accessible outside of Engine.

Basically, in the situation you described, I'm advocating for you to think of Engine as a specialized container that holds a Scene, among the other things Engine might hold and do, publicly and privately.

void clientCode(Engine &engine, std::vector<std::string> &strArray)
{
    strArray.back() = "As a container, I manage the lifetime of certain variables, providing public access to some of my members - either through functions, or directly.";

    engine.scene.InitSkyBox("Hey, as an 'Engine', I do alot of similar features. I'm kinda like a container also... but more specialized, with certain algorithms built in to operate on my variables. Algorithms like how std::map has std::map::erase and std::map::equal_range");
}

However, if Engine only ever has one scene at a time (since you have it in a unique_ptr), why not make it a non-pointer member variable?
Also, unless Engine needs to do special things when the scene is modified (unlikely, since you access the pointer from outside of Engine), why not have it public?

I used unique_ptr to express ownership in the example (or if scene was polymorphic), but it would be similar to having it as value type and GetScene() returning a reference.

It doesn't need to be named 'current' if it's the only one.
It doesn't need to be private if you access it from outside of Engine anyway, and if outside modifications don't upset Engine.
It doesn't need to be a pointer unless you need it dynamically allocated, and if you do need it dynamically allocated, those allocations should probably happen inside of Scene anyway, not by classes using Scene.

That what I'm wondering. If I'm a OO taliban, would engine be the only guy who can manipulate a scene ?

Do you do this in your code?
std::unique_ptr myString;
std::string manages its memory itself (aside from small-string optimizations), you don't need to have all your strings be wrapped in smart pointers. Sometimes you do, for memory or performance reasons, but in the general case, let std::string manage itself. In the general case, 'Scene' should manage itself.

No, I only use unique_ptr for polymorphism and pointer ownership.

Having Scene private inside Engine is an illusion if you pass around non-const pointers to it - might as well make it public!
Unless, you actually need to have Engine adjust, constrain, or otherwise be aware of changes to Scene, in which case you can't help but keep Scene private and replicate most of Scene's interface in Engine and keep Scene only const-accessible outside of Engine.

Yeah that's what I thought but it sounds evil. Overall it seems if I want to manipulate a scene from another object, I have to either make it public (or returning a reference / pointer), or duplicate all of Scene methods into Engine for delegation.

If you look at it another way and take the approach that you should be injecting objects instead of having objects create other objects, then Engine wouldn't own Scene but simply use it. You would pass a Scene object to the Engine ctor. You could make an IScene interface that Engine takes and uses. Let the user own the scene object.

void UserCode()
{
    IScene* myScene = new Scene("file.map")
    Engine engine(myScene);
}

Or if you scene needs the engine to load (guessing it would) you could pass a ref of the engine to the scene or use pointers to an engine interface. Now you're coding to an interface. You can let the users own these objects and instead of storing them in other objects just pass them around when needed.

void UserCode()
{
    IEngine* engine = new CoolEngine();
 
    IScene* myScene = new Scene(engine, "file.map");
}

I'm always asking myself if I need to actually store objects inside other objects or can I just pass them as parameters when they are needed.

Maybe some day you want to be able to load multiple scenes in another thread to make a seamless world.

I would consider storing objects inside other objects only IF the container object doesn't need to expose them otherwise like you said, you're sort of breaking encapsulation by just providing an internal object as is.