I've been refactoring the codebase for the engine I've been working on for the past several years, and have realized I may have made a grave architectural mistake. I started out by going back and adding const in areas that I had ignored prior just to get something working…which led me to eventually question the fact that I'm returning non-const pointers to private class members from a couple of my classes. Before I explain any further the following is a graph I made which outlines all Classes and their members where `App` is a singleton which encapsulates all logic and controls the loop: https://stashcube.com/vel-flowchart.pdf​

So for example my Scene class contains `std::vector` members for all assets which are used for that scene instance. I then provide methods which dereference positions in those vectors to return raw pointers to individual assets. This allows for these values to be stored contiguously in memory. The raw pointers are then used by other elements (all of which would go out of scope prior to Scene).

However, this breaks encapsulation as these vectors are declared as private members within Scene as I do not wish for values to be added without going through a setter as additional logic is required for some at the time they are added into their corresponding containers. Below is my header file for a Scene:

namespace vel
{
	class Scene
	{
	private:
		std::vector<Shader>					shaders;
		std::vector<Mesh>					meshes;
		std::vector<Texture>				textures;
		std::vector<Material>				materials;
		std::vector<Animation>				animations;
		std::vector<Renderable>				renderables;
		std::vector<Armature>				armatures;
		std::vector<Stage>					stages;

		double								animationTime;


	protected:
		size_t								loadShader(std::string name, std::string vertFile, std::string fragFile);
		void								loadMesh(std::string path);
		size_t								loadTexture(std::string name, std::string type, std::string path, std::vector<std::string> mips = std::vector<std::string>());
		void								loadSceneConfig(std::string path);

		size_t								addMaterial(Material m);
		size_t								addRenderable(std::string name, size_t defaultShaderIndex, size_t crateMeshIndex, size_t crateMaterialIndex);
		Stage&								addStage();

		Renderable							getRenderable(std::string name);
		Armature							getArmature(std::string name);
		Stage&								getStage(size_t index);


	public:
		Scene();
		~Scene();
		virtual void						load() = 0;
		virtual void						innerLoop(float deltaTime) = 0;
		virtual void						outerLoop(float frameTime, float renderLerpInterval) = 0;
		virtual void						postPhysics(float deltaTime);

		bool								loaded;
		
		Mesh*								addMesh(Mesh m);
		Animation*							addAnimation(Animation a);
		Armature*							addArmature(Armature a);

		Shader*								getShader(size_t si);
		Shader*								getShader(std::string shaderName);
		Texture*							getTexture(size_t textureIndex);
		Material*							getMaterial(size_t materialIndex);
		Material*							getMaterial(std::string materialName);
		Mesh*								getMesh(size_t index);
		const std::vector<Mesh>&			getMeshes() const;
		Animation&							getAnimation(size_t index);
		std::vector<Animation>&				getAnimations();
		
		size_t								getShaderIndex(std::string shaderName);
		size_t								getTextureIndex(std::string textureName);
		size_t								getMaterialIndex(std::string materialName);
		size_t								getMeshIndex(std::string meshName);

		void								updateAnimations(double runTime);
		void								draw(float alpha);
		void								stepPhysics(float delta);
		void								applyTransformations();
		void								processSensors();

	};

}

Now other than this being a rather nasty code smell, everything in my existing source works, and I have come quite a long way. BUT now that I've realized what I've done, I'm trying to figure out what the best way would be to fix or restructure my existing design architecture so that I'm properly using const everywhere I need to be AND not breaking encapsulation, BUT keeping my existing OOP layout…but that might not be feasible?

I suppose if I made these raw pointer return values T* const that would effectively correct the encapsulation issue, but the problem with that is that I may need to modify a Mesh at some point by something other than Scene.

Was hoping to get some peoples perspective on the situation. Maybe some opinions or options as to any better ways to structure what I'm doing. Any/All replies appreciated! ?

Const correctness … yeah … difficult. One the one hand it higly depends on the level of security as I think it is just a compile-time security feature (in the end, a const pointer doesn't realy differ from a non-const pointer), on the other hand it also depends on who will use the code. My usual answer to the question is to make properties const which should be used from const references, mostly getters and if the object should be somehow modified, don't pass it as const reference.

I'm from a C# background (learned that in school in early 2000s) so I don't think in “getter/setter” methods but “properties”. That's why my “getter/setter” methods are always named the same but differ in their parameters and return type:

force_inline T* Data() //Getter
force_inline const T* Data() const //const reference Getter 
force_inline void Data(T* value) //setter

And all of my functions follow the same structure if a parameter could be modified, I pass it as reference or pointer or else const reference:

varying DoSomething(T& param) //pass by reference; requires not null, can be modified
varying DoSomething(T const& param) //pass by reference; requires not null, won't be modified
varying DoSomething(T* param) //pass by pointer, can be null, may be modified
varying DoSomething(T const* param) //pass by pointer, can be null, won't be modified

So this is how my code usually looks like and it helped me a lot to design a good looking user interface to our engine, without any strange naming (looking at your Unreal Engine <.<) or the need to crawl through massive amounts of documentation.

Other than that, I won't spend too much time into trying to catch someone else's faults. A user can always const_cast your std::vector to manipulate it's contents and you can't prevent that unless you hide it in some kind of class which does not allow direct access to the underlaying data but has functions to do so.

My philosophy is to make it hard to fail but also easy to do things right if you need to, always with the sentence in mind “hey user, you might do that but be aware of possible consequences!”. Our data classes are all inheriting from our own Array implementation. So data can be accessed as an Array if you need it in debugging or serialization for example. This is a design choice I made and it enables a lot of possibilities. Our JSON class for example can be used with any function, which takes an Array as an input parameter

Thanks for the response! I'm thinking I might have a solution. So instead of having the scene hold these containers, I could move them in my App Singleton as public members. This would then at least read correctly since objects other than a scene will need to manipulate these values. This would also allow for faster load times since I could then keep assets loaded between scene swaps that are used in both scenes. The scene class would then provide the same interface, only storing data in the containers now belonging to the App Singleton and track which assets in App it's using (so when a new scene is loaded we know what to keep and what to delete).

Then we would need to note that assets should not be loaded into the App containers directly unless the individual using the API knows exactly why they're doing what they're doing.

Sound like I'm on the right track?

whitwhoa said:
track which assets in App it's using

Why do you keep those assets in an std::vector of the global App rather than store them in some kind of asset manager? I mean, wouldn't it be easier to let an asset manager hold some portion of the heap memory and not just add assets needed to be loaded but also perform some kind of ref-counting on them? You then could simply request an Asset Pointer from that manager and have it decrease the reference count if it goes out of scope. Then you won't need to worry about someone adding stuff to your std::vector which isn't done from within certain function

I like the idea of the asset manager. However, (and I may be misunderstanding something, so bear with me) if I were to create an asset manager class and move my vector containers into this class as private members and only allow modifications via member methods, some of which return shared_ptrs, and then those shared_ptrs are saved as members of other classes and used to modify the data stored within the asset manager…would that not just be a fancier version of my current debacle? If I didn't make them private they could still be accessed directly. But then again, like I said I may not be "picking up what you're putting down" ?

Side note: I found this today and am considering using it instead of std::vector for my containers…still going through it at the moment. Looks promising (as a replacement for vectors in certain situations)

The good news is that all these tools are there to help enforce correct use.

The use can be correct without any safeguards. Just look at older c code or assembly code to see how it is about policy and proper use rather than language-enforced use.

As you gain experience you gain new tools and skills. Resource management systems are one tool. Const correctness is a tool. Proxy objects are a tool. Smart pointers and container classes are tools. All of these communicate intentions, and all can help ensure policy is followed, and all can be violated, ignored, or misused.

When I look at your code snippet personally I am more concerned about the public virtual functions and their potential for abuse, as I know the ones you mentioned are simple policy issues.

That said, the answer to the question asked is to just do it. You have a better tool now, so start using it.

If you're the only one in the code, change it all. If others are using the code migration can be more complex, ranging from email announcements to gradual depreciation with migration documentation. Either way, use the best tool you have.

frob said:

That said, the answer to the question asked is to just do it. You have a better tool now, so start using it.

Just do it as in build the asset manager class and allow it to return the pointers to the private members? That's kind of the way I'm leaning. That way I can note the functionality in that class and explain that this is by design.

First: This sounds like a code implementation question, not an architecture question.

Second, let me make sure I understand you correctly. You have something like:

class MyScene {
    std::vector<Texture *> textures_;
public:
    Texture *getTexture(uint index) const { return textures_[index]; }
};

(Clearly, you will do range checking, and maybe get textures by name instead of index, and so on – that's not the important part here.)

Now, does returning the value (of type Texture *) stored inside your private container (textures_) “break encapsulation”? No. No, it doesn't. You're managing Texture* values, and you're not changing those values inside a const function. “const" is not (necessarily) transitive – if someone wants to mutate the innards of that Texture, that's not necessarily a concern of your MyScene interface.

Consider this: What if these were colors, returned by value?

class MyScene {
    std::vector<Color> colors_;
public:
    Color getColor(uint index) const { return colors_[index]; }
};

Now, would you say that getColor() breaks encapsulation? Well, no, it's just an interface dealing in “Color” values that your class manages.

Similarly, your class manages values of type “Texture *” and has interfaces for those values.

Separately it might not be the best idea (depending on how you do other things) to deal with assets using raw pointers, rather than using some kind of smart pointer. Modern C++ really, really, wants to use some kind of smart pointer to manage lifetime of objects. Thus, you might separately want to do something like this:

class MyScene {
    std::vector<owning_ptr<Texture>> textures_;
public:
    using_ptr<Texture> getTexture(uint index) const { return textures_[index].getUsingPtr(); }
};

You might even want to know who uses each asset, to do garbage collection, runtime patch-up of replacements (very handy in editors) and so on:

class MyScene {
    std::vector<owning_ptr<Texture>> textures_;
public:
    using_ptr<Texture> getTexture(uint index, IAssetUser *user) { return textures_[index].getUsingPtr(user); }
};

Now, you don't make this change to “change the architecture of your MyScene.” You make this change because you want a better handle on usage versus ownership of assets.

Also, there might very well be two or more layers of asset ownership.

• One layer might be “how do I find and read a file from storage?” This deals with file systems, memory mapped files, archive parsing, and so on.
• One layer might be “how do I load these bits into a GPU such that I can use them?" This deals with format conversion, texture handles, vertex buffer assignments, and so on.
• One layer might be “how does a particular object decide how to render itself?” This deals with references between game objects and assets, texture-swapping customizations, how game code can provide parameters to shaders, and so on.

This separation into what the layers are, feels much more like what “engine architecture” is to me, rather than “where do I use a pointer versus a reference versus a wrapper object versus a smart reference?" which is more of just a code implementation concern. Architecture can be ported between languages (C++, Python, Rust, doesn't matter) whereas implementation details are usually language and API specific.

Assuming you want this level of separation, you'll probably want one “I/O system” which can find and cache bits from disk/archive/network as needed (probably with an asynchronous API.) There is only one I/O bus (typically) so a single global manager makes sense. You can have one “asset loader” which owns the graphics-API level identifiers (ID3DTexture2D or GLuint or whatever) if you only ever create one graphics context, although you might want more than one for fancy multi-head type cases, or if you build something like a CAD app with many different modes. Finally, you could have many “scenes” that each manage which assets are used in the particular scene, because you load many scenes. These different layers solve different problems, and you're likely to be better off choosing the right representation for each concern.

hplus0603 said:
Second, let me make sure I understand you correctly. You have something like:

class MyScene { 
	std::vector<Texture*> textures_; 
	public: 
		Texture* getTexture(uint index) const { 
			return textures_[index]; 
		} 
};

Actually what I have is the following:

class MyScene { 
	std::vector<Texture> textures_; 
	public: 
		Texture* getTexture(uint index) const { 
			return &textures_[index]; 
		} 
};

So the getter is dereferencing the value of the vector location to return a pointer to that specific element. I'm doing this vs the vector of pointers in an effort to keep values as contiguous as possible.

whitwhoa said:
would that not just be a fancier version of my current debacle?

Not inevitable. Using shared_ptr is a case of being dependent on it's implementation, I'd make my own “ResourceHandle” and return that instead. This way you can also make access to the underlaying pointer const and const might not be modified that easy. And if someone still modifies it, then it happens on own risk!

There was a discussion about resource management back a few months in this topic https://www.gamedev.net/forums/topic/709073-resource-manager-design-using-store-loader-cache-and-proxy-concepts/​

whitwhoa said:
Side note: I found this today and am considering using it instead of std::vector for my containers

Looks ok, another option is to use a robin hood hash map as it is also considered to be very fast (2.4 micro-seconds in C# vs. 2 milliseconds to access a generic Dictionary<T, T>, just to some numbers on hand) and it can be made of a single continous block of memory