Sign in to follow this  
schupf

Create objects only once

Recommended Posts

Hello,

I have a little design problem.

I have a Material class, that always has a VertexShader and PixelShader and additionally tons of states (Textures, Culling etc.)
The problem is: Many objects will use the same Material but the Material should only created ONCE (the first time the Material is acquired) and all
other requests to that material the already created Material should just be returned.

The only way I can think of is this:
[code]
Material* mat = manager->getMaterial("MyMaterial");
if(mat == NULL) {
// This is the first time someone wants to use this material. So create it
mat = manager->getMaterial("MyMaterial");
VertexShader* vs = manager->loadVertexShader("shader.vsh");
PixelShader* ps = manager->loadVertexShader("shader.psh");
mat->setVS(vs);
mat->setPS(ps);
mat->setTexture(0, myTexture);
object->setMaterial(mat);
}
else {
object->setMaterial(mat);
}

[/code]
The problem is: I have to use this whole code block EVERYTIME I need this material. Cause I can never be sure if the material is already loaded.
This is really ugly:(

Does anyone has an idea how I could simplify this?

Share this post


Link to post
Share on other sites
If you need to do it every time, the obvious solution is to create a function that contains that functionality.

Share this post


Link to post
Share on other sites
I usually do something like:


[code]
class FooCache {
public:
Foo* get(std::string name)
{
std::map<std::string, Foo*> result = foomap.find(name);
if(result != foomap.end())
{
return (*result).second;
}
else
{
Foo *tmp = loadFoo(name);
foomap[name] = tmp;
return tmp;
}
}
private:
std::map<std::string, Foo*> foomap;
};
[/code]

So get will load the object only once.

Edit: Why are the code tags always messing up the indentation?

Share this post


Link to post
Share on other sites
@japro: Actually I exactly have the same design, but that does not solve my problem. What if loadFoo() needs more parameters? Then you have to pass them to get() EVERY time, which is kinda ugly. Plus what if the returned Foo needs additional configuration after get (your Foo is like my Material), but only once?

Share this post


Link to post
Share on other sites
[quote name='schupf' timestamp='1312735613' post='4845816']
@japro: Actually I exactly have the same design, but that does not solve my problem. What if loadFoo() needs more parameters? Then you have to pass them to get() EVERY time, which is kinda ugly. Plus what if the returned Foo needs additional configuration after get (your Foo is like my Material), but only once?
[/quote]
What would be an example where load needs more parameters? Either those parameters belong to the object in which case they should be loaded together with it, if they are global parameters (load every texture into this type of format) then that should be a parameter of the cache itself that you set with some "FooCache::setParameter" method globally or the parameters are situation dependent in that case they should be somehow passed to the Foo object itself or be applied via some sort of decorator pattern and not be passed to its cache. At least these are the cases I can think of.

Share this post


Link to post
Share on other sites
[quote name='japro' timestamp='1312736481' post='4845822']
[quote name='schupf' timestamp='1312735613' post='4845816']
@japro: Actually I exactly have the same design, but that does not solve my problem. What if loadFoo() needs more parameters? Then you have to pass them to get() EVERY time, which is kinda ugly. Plus what if the returned Foo needs additional configuration after get (your Foo is like my Material), but only once?
[/quote]
What would be an example where load needs more parameters? Either those parameters belong to the object in which case they should be loaded together with it, if they are global parameters (load every texture into this type of format) then that should be a parameter of the cache itself that you set with some "FooCache::setParameter" method globally or the parameters are situation dependent in that case they should be somehow passed to the Foo object itself or be applied via some sort of decorator pattern and not be passed to its cache. At least these are the cases I can think of.
[/quote]
For example loadShader(). My loadShader() method looks like this:
ShaderPtr loadShader(const string& filename, const string& entryFunction);

Usage:
obj->VS = manager->loadShader("myShader.vsh", "main");
In order to load (which involves compiling) the shader I need the name of the entry function (cause D3DXCompileShaderFromFile needs it). Its kinda awkward to pass the function name everytime I need the shader (even though actually I only need it for the first time)

Share this post


Link to post
Share on other sites
Then either make sure that the entry function is always called "main" and have loadShader always pass it to the DX call, or write some "meta" file that contains all the information to load that specific material. I.e. when the material consists of two shader sources and a texture make a file "wood.material" or something that contains all the information (entry function, names of the shader files, name of the texture).

Share this post


Link to post
Share on other sites
Nitpicking on style:
[code]Foo* get(std::string name)
{
std::map<std::string, Foo*> result = foomap.find(name);
if (result == foomap.end()) {
result = foomap.insert(result, make_pair(name, loadFoo(name)));
}
return result->second;

}
[/code]

Technically, this is less efficient since it's pessimistic (item is expected to not be in cache) while CPU branch prediction expects optimistic branch (if statement should favor common case).

But since map doesn't allow fast query (such as hash comparisons for approximate match) and due to other implementation details it's a minor point, just something to be aware of.


Alternate reusable version:[code]
template < class K, class T, class C, class A, class Factory >
T get_cached(std::map<K,T,C,A> & map, K key, Factory factory) {
typename std::map<K,T,C,A>::iterator = map.find(name);
if (result == map.end()) {
result = foomap.insert(result, make_pair(name, factory(key)));
}
return result->second; }

...
// example factory
Foo * file_loader(string filename) {
// load from file
return new Foo(file_data);
}
[/code]

I'm not entirely sure if this falls under AOP or mixins or similar, but one augments an existing class with completely orthogonal functionality. get_cached allows any map to now act as cache simply by providing a factory function.

Approach is especially useful when working with third-party code where modification of existing classes isn't possible or desirable.

[quote]What if loadFoo() needs more parameters?[/quote]
If parameters don't change between invocations, then consider the Factory example above:
[code]
struct StatefulLoaderFactory {
StatefulLoaderFactory(X p1, X p2, X p3);

Foo * operator()(filename f) {
do_the_load(filename, param1, param2, param3);
}
private:
X param1;
X param2;
X param3;
};

...
StatefulLoaderFactory f(10, 200, 55);
...
// works because of templates:
get_cached(map, "some_file.txt", f);[/code]

Share this post


Link to post
Share on other sites
[code]
Material* mat = manager->getMaterial("MyMaterial");
if(mat == NULL) {
// This is the first time someone wants to use this material. So create it
[/code]

Don't do that.

You're putting in a branch point you don't need. I presume the second call to "getMaterial" should be "createMaterial". Also, you're not checking the error states of anything.

You're putting loads in unpredictable places. You now have several possible disk accesses (which are slow) plus shader compiles inside your rendering cycle. It means the first time a given material appears in view you're going to get chunky frame times.

Just go and load up all the stuff you need before you start. Presumably you are loading up the models anyway which name the material types, so just stick them in a list, then create them all. Then you can go through all the models setting the material pointer. Then you can start rendering, and now you know that all the bits are in place, so you don't need to worry about getting nulls back from your getMaterial call.

Don't put ANYTHING you don't need in your render loops -- anything you can do upfront, do it upfront.

Share this post


Link to post
Share on other sites

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