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.

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