Sign in to follow this  

Writing code more general

This topic is 1105 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hey,

 

so I found that in a few classes of mine, the code might not be as easy to reuse as I would like it to be. In this case, I found quite a few instances where I define enums, which might change later on. So I figured I could rewrite the whole thing and move the enums out of the class, heres my idea in combination with a meshloader:

#include <string>
#include <unordered_map>

//General plugin class
template<class PLUGIN_ENUM, class PLUGIN_INTERFACE>
class IPluginContainer
{
public:
	typedef PLUGIN_ENUM PluginEnum;
	typedef PLUGIN_INTERFACE PluginInterface;

	bool addPlugin(PluginInterface* plugin)
	{
		//insert type/plugin pair into pluginMap
		PluginEnum type = plugin->pluginType;
		return true;
	}

protected:
	PluginInterface* getInterface(PluginEnum type);

private:
	std::unordered_map<PluginEnum, PluginInterface*> pluginMap;
};

//Interface for object which want to be added to the plugincontainer
template<class PLUGIN_ENUM>
class IPlugin
{
public:
	IPlugin(PLUGIN_ENUM pluginType) : pluginType(pluginType) {}

	IPlugin() = delete;

	const PLUGIN_ENUM pluginType;
};

//currently supported mesh creation types
enum class MESH_LOADER_TYPES
{
	ASSIMP,
	HEIGHTMAP
};

//interface meshloader
class IMeshLoaderPlugin : public IPlugin<MESH_LOADER_TYPES>
{
public:
	IMeshLoaderPlugin(MESH_LOADER_TYPES pluginType) : IPlugin(pluginType) {}
	IMeshLoaderPlugin() = delete;

	virtual bool loadFile(std::wstring filename, /*buffer stuff,*/ void* customData) const = 0;
};

//creates heightmaps from rawfiles
class HeightmapLoader : public IMeshLoaderPlugin
{
public:
	HeightmapLoader() : IMeshLoaderPlugin(MESH_LOADER_TYPES::HEIGHTMAP) {}

	struct HeightmapDesc
	{
		double yScale;
	};

	bool loadFile(std::wstring filename, /*buffer stuff,*/ void* customData) const override
	{
		//cast customData to HeightmapDesc
		//create mesh and put it into passed buffer
		return true;
	}
};

//pass data to assimp
class AssimpLoader : public IMeshLoaderPlugin
{
public:
	AssimpLoader() : IMeshLoaderPlugin(MESH_LOADER_TYPES::ASSIMP) {}

	struct AssimpDesc
	{
		int someFlag;
	};

	bool loadFile(std::wstring filename, /*buffer stuff,*/ void* customData) const override
	{
		//cast customData to AssimpDesc
		//call assimp lib
		//create mesh and put it into passed buffer
		return true;
	}
};

//the actual meshloader
template<class T_MESH_LOADER_TYPES>
class MeshLoader : public IPluginContainer<T_MESH_LOADER_TYPES, IMeshLoaderPlugin>
{
public:
	bool load(PluginEnum type, std::wstring filename, /*buffer stuff,*/ void* customData) const
	{
		//get loader from IPluginContainer: type->loader
		//call loader and return buffer stuff
		return true;
	}
};

int main()
{
	MeshLoader<MESH_LOADER_TYPES> loader;
	AssimpLoader* assimpLoader = new AssimpLoader();
	HeightmapLoader* heightmapLoader = new HeightmapLoader();

	loader.addPlugin(assimpLoader);
	loader.addPlugin(heightmapLoader);

	AssimpLoader::AssimpDesc ad;
	HeightmapLoader::HeightmapDesc hd;
		
	loader.load(MeshLoader<MESH_LOADER_TYPES>::PluginEnum::ASSIMP, L"somefile.raw", /*buffer stuff, */ &ad);
	loader.load(MeshLoader<MESH_LOADER_TYPES>::PluginEnum::HEIGHTMAP, L"somefile.raw", /*buffer stuff, */ &hd);

	return 0;
}

1) Well, first off I would like to know if this is a good idea in general, my plan is to make two projects, and the meshloader would be in some kind of an engine project, which I cant change because it will be used for 2 different projects, which will use different loaders and who knows what else might be added later.

 

2) The void pointer in the load method, I don't like it, but I don't see any other way. Line 52/66/85

 

3) Where would you store the enum definition, I actually want to use this quite a few times and there will be other unrelated enum definitions which need to be stored somewhere.

 

Thx in advance

Epi

Edited by BloodyEpi

Share this post


Link to post
Share on other sites
I'm not sure enums are the right thing to use in this particular case. It looks like you want to use unique IDs and enums are (one) convenient method to auto-generate unique IDs. Assuming you don't mind them changing on you if you edit the enum "wrong" and don't mind re-compiling every time you want to adjust the enum.

I suggest you look into using an actual ID system and assert/throw an exception if you get duplicate ID numbers for different plugins or interfaces.

This can be done with a simple 32-bit integer (or larger if you're going for some sort of "globally unique" system if you expect users to generate their own plugins - in which case you should probably look into GUIDs) but even then I'd suggest a tiny wrapper class around the ID to prevent people from passing you random integers without a compiler error.

Heck, you could get fancy and make your ID class hash a unique string and use that as the ID. Of course, you still may have to deal with hash collisions, but again, you can catch that when you "register" the ID in your unordered_map.
 
class PluginID
{
public:
  // Explicit makes sure that you can't pass strings and integers to functions expecting IDs
  // without invoking the PluginID constructor explicitly
  explicit PluginID(const char* IdString): _ID(some_hash_function(IdString)) {}
  explicit PluginID(unsigned int ID): _ID(ID) {}

  // We want to be copyable, but we don't need anything fancier then the default
  PluginID(const PluginID& Other) = default;
  PluginID& operator=(const PluginID& Other) = default;

  bool operator==(const PluginID& Other) const {return _ID == Other._ID;}
  bool operator!=(const PluginID& Other) const {return !((*this) == Other);}

  // Other operators if necessary - you shouldn't ever need to get at the "real" ID though

private:
  unsigned int _ID;
};
Edited by SmkViper

Share this post


Link to post
Share on other sites

The void pointer thing is troublesome. I may be missing some of the fine points, but to me, the point of having an interface like IMeshLoaderPlugin, and a method like IMeshLoaderPlugin::loadFile, is that, given an IMeshLoaderPlugin, you can call loadFile() without knowing what kind of plugin you have. In this case, that is lost; if you have an assimp plugin, you have to give it an AssimpDesc or it will fail. Worse, it will fail at run-time, not at compile-time.

 

So, my question to you: Is there some good reason to have a generic IMeshLoaderPlugin::loadFile method, and the related IMeshLoader::load function? Why not just have separate AssimpLoader::loadAssimpFile and HeightmapLoader::loadHeightmapFile functions? You may well be getting some other benefit from the generic IMeshLoaderPlugin::loadFile method that I'm just not seeing. However, if I were you, I'd be looking at my class abstraction carefully. If you really have plugins that are not interchangeable, then you probably don't need any of this enum stuff in the first place.

 

Anyways, I hope that's a little helpful. I've probably asked more questions than supplied answers. Good luck!

Geoff

Share this post


Link to post
Share on other sites

That's actually a good point. After adding a hashid(thx smkviper, Ill still need this in other places:)), I thought about my design and found that I pretty much just wrote the interface to store all loaders in one container, but only need them during loading times, so I don't even need to store them permanently. I guess removing the interface and MeshLoader class would be a good idea.

 

Thanks to you both.

Share this post


Link to post
Share on other sites

This topic is 1105 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this