MapExpressions

[greebo]

The processed image is only requested once, in GLTextureManager::getBinding(), which calls the TextureConstructor::construct() method. The TextureConstructor allocates the memory for the Image object, writes the pixel data (in whatever complicated way) and returns the readily fabricated Image. The GLTextureManager loads the stuff into OpenGL memory and immediately releases the Image object from memory again. So basically, there is no need to save the actual Image data in any of these MapExpression classes. (Sorry if I'm repeating stuff you might already know ).

 

As for changing other classes or interfaces: if you feel that this is necessary, nobody is holding you back.

 

You will probably need to change the implementation of CShader::getBump(), CShader::getDiffuse() and such. Currently, these just create a DefaultConstructor object which loads the actual file from the disk. Instead of

TextureConstructorPtr cons = TextureConstructorPtr(new DefaultConstructor(_bumpName));

this should be

TextureConstructorPtr cons = _bumpMapExpression->getImage();

with _bumpMapExpression as the top-level MapExpression object with all its recursions.

 

Personally, I would prefer if the MapExpression::getImage() method was named MapExpression::getConstructor(), because it's not delivering an actual image, but a TextureConstructor.

 

Hopefully, I didn't confuse things more than clarifying them?

[greebo]

Hm, thinking about this, it maybe isn't necessary to keep that TextureConstructor class. If the MapExpression object is capable of delivering the image data, that's fine as well. This could also happen recursively, so there's probably no reason to complicate things by another middleman class.

 

So, you could probably change the GLTextureManager::getBinding() method to receive an MapExpression object instead of a TextureConstructor.

 

The call stack would be:

ShaderCache requires bumpmap image >> CShader::getBump() >> GLTextureManager::getBinding(IMapExpress

ionPtr expr) >> calls MapExpression::getImage() >> recursions >> Image is delivered.

 

[edit] That damn forum software is adding a line break where I don't want it...

 

Btw: you can probably create a shortcut typedef

typedef boost::shared_ptr<IMapExpression> IMapExpressionPtr;

to save some typing.

[mohij]

Bypassing the TextureConstructor class would mean returning directly an image object?

 

Just out of curiosity, would I have had to implement my own version of the textureConstructor class? I only found two implementations that both load from filename.

 

Is the TextureConstructor class still needed when MapExpressions directly produce image classes?

 

When the TextureConstructor is only needed for the MapExpressions (that's what it looks like atm), then I would probably remove it.

[greebo]

Bypassing the TextureConstructor class would mean returning directly an image object?

Yes, the MapExpression class would return the Image.

 

I think we can't implement the recursive image processing when using TextureConstructors. I thought about a nested addnormals(heightmap(texture/blah), texture/blah2) expression. With the above class inheritance, this would mean that there is one "master" MapExpression containing two child MapExpressions. The master is the one that is contained within the CShader class as _bumpMapExpression and is passed to the GLTextureManager::getBinding(). The GLTextureManager calls getImage() on the master expression, which would have to query its two child objects for their images and to subsequently merge the two textures into one single texture.

 

At the end of the inheritance chain MapExpressions will of course have to invoke some kind of image loader, but this doesn't have to be encapsulated within a TextureConstructor, like it is now.

 

Long text, short conclusion: I think we can ditch the TextureConstructor mechanism in favour of nested MapExpression::getImage() methods.

 

Just out of curiosity, would I have had to implement my own version of the textureConstructor class? I only found two implementations that both load from filename.

Yes, this will be necessary, I guess. There will be need for MapExpressions loading files from the Virtual File System, but some high-level MapExpressions will need to implement or call image processing algorithms. Maybe these algorithms can be outsourced into static methods of a TextureManipulator service class, like the one I attached to the GLTextureManager.

[mohij]

TextureConstructorPtr cons = _bumpMapExpression->getImage();

Shouldn't that be:

TextureConstructorPtr cons = IMapExpression->createForToken(tok)->getImage();

and now that we discussed it, with an Image instead of Textureconstructor.

[greebo]

Well, if we ditch that TextureConstructorPtr, it probably should be like this:

Image* (or ImagePtr) img = _bumpMapExpression->getImage();

IMapExpression->getImage() won't work, because there is nothing to dereference here, you'll need a pointer to the class instance, hence _bumpMapExpression, which I implied that it should be declared as a class member of CShader:

class CShader
{
  IMapExpressionPtr _bumpMapExpression;
public:
  ....
};

 

edit: Ah, I see the edit now. This won't work either, because you need to call it like this:

IMapExpression::createForToken(tok)->getImage();

(A static method is to be treated like a global function in a namespace, hence IMapExpression::createForToken()).

But I would still vote for saving the IMapExpressonPtr as a local member of CShader, so it can be re-used without having to parse the material files again.

[mohij]

Yes, understood.

What do you think is better, first changing the surrounding files to work with the new MapExpressions and then implementing them (makes testing easy) or other way round?

[OrbWeaver]

What do you think is better, first changing the surrounding files to work with the new MapExpressions and then implementing them (makes testing easy) or other way round?

 

You should make any changes necessary in your local workspace in order to get the implementation into a state where you can test it and confirm it to be working successfully, then use svn diff to produce a patch and send it to us.

[greebo]

If I were to do it, I would implement a minimal MapExpression implementation first and try to adapt the surroundings to use it (i.e. CShader and GLTextureManager). Whatever feels comfortable to you is good enough.

 

If you're aiming for a seamless transition you could overload the GLTextureManager::getBinding() method with another variant, taking an IMapExpressionPtr as argument (instead of a TextureConstructor). This way you could use both approaches without breaking the other system. Then try to migrate the CShader::getTexture() method to send IMapExpressionPtrs to the GLTextureManager, using a barebone MapExpression, just loading a file from the disk. Once this is done, you could proceed with making the system more intelligent.

 

edit: whoah, once again, OrbWeaver was faster. Yes, as soon as you've got a compiling version at hand, we can have a look at it.

[mohij]

That's not exactly what I meant, but I will just continue with the MapExpressions themselves.

[greebo]

Whoops, sorry, what were you meaning then?

[mohij]

Hehe, same-time post, the reply was meant for OrbWeaver. Your reply wasn't there when I started the reply. Sorry for the confusion.

[OrbWeaver]

Oh, I see what you mean now -- is it easier to change the calling code to REQUIRE MapExpressions which won't initially work until you implement them, or to implement the MapExpressions first and then splice them into the calling code.

 

Generally I tend to go with the first approach: decide what you want to happen, then write the code that makes it happen. This is sort of like test-driven development but without the formal processes.

[mohij]

class IMapExpression {
	   Image result; /* will hold the image on first call of getImage(), further calls to getImage will simply return result */
   public:
	   static boost::shared_ptr<IMapExpression> createForToken(parser::DefTokeniser& token);
	   virtual Image& getImage() = 0;
   };

 

Last question: Is this interface fine now (before I start to change everything to work with this one)?

[OrbWeaver]

Don't put the Image data member as part of the interface, this would be a separate part of each implementing class. Some classes may not even want to store the image, they could create it on demand and return it there and then.

 

It would also be better if the getImage() method returns a shared_ptr<Image> (typedef'd as ImagePtr) rather than a reference to an existing object.

[greebo]

The interface is fine, I think.

 

I'm not sure whether saving the image object as copy is a good idea, because it will moved into OpenGL memory once and after that it's not needed anymore. The GLTextureManager is deleting the image after loading it into OpenGL anyway, so the MapExpression just needs to allocate it, to fill it and to deliver the pointer to it. Otherwise it would stay on the heap (textures can take up several dozens to hundreds of MB quickly).

 

edit: Argh! Next time I'm gonna wait five minutes before I start typing.

[mohij]

So best is not to have any data in there at all, ok.

[greebo]

It's best to leave any data members out of the abstract base class definitions, because it only limits the freedom of implementation. It should completely be up to the subclasses how they implement this definition.

[OrbWeaver]

Yep, an interface class should only contain the methods that are part of the public interface for the objects, any private data fields or helper methods should be left out. An interface can sometimes contain constants or typedefs, for example std::string::npos.

[mohij]

I finished the minimal MapExpressions.

MapExpressionnew.cpp

MapExpressionnew.h

You can take a look if you want.

[mohij]

In ShaderTemplate.cpp the constructNull function is used twice, should I implement that one too or change ShaderTemplate?

[greebo]

I can't compile it right now, but from reading the code, it looks good.

 

Does this version compile? Because I noticed the getImage() method returns a reference Image&, whereas the actual implementation returns a pointer.

 

Also, I saw that you're returning this:

return shared_ptr<IMapExpression> p(new MakeAlphaExpression (token));

which could be made simpler, without constructing an intermediate pointer object p:

return shared_ptr<IMapExpression>(new MakeAlphaExpression(token));

I personally would prefer to use a typedef'd IMapExpressionPtr instead of the shared_ptr, but that's not a real concern, more or less a matter of taste.

 

So far, so good. Did you already have a look at the ShaderFileLoader (the parser)? That's probably the point where these should be hooked in.

 

In ShaderTemplate.cpp the constructNull function is used twice, should I implement that one too or change ShaderTemplate?

I can't recall right now what this function does, maybe OrbWeaver can tell you more about this?

[OrbWeaver]

I can't recall right now what this function does, maybe OrbWeaver can tell you more about this?

 

No idea I'm afraid, it's almost certainly legacy cruft that doesn't needs to be there. Properly-designed C++ classes are unlikely to have a function called constructNull(), so there should be no problem removing it if there is a better implementation available.

[mohij]

greebo: I didn't try to compile it when I posted the links, but since then I'm on fixing compile errors, got quite a lot. Now it compiles. And I also changed some typedefs. About the p in

return shared_ptr<IMapExpression> p(new MakeAlphaExpression (token));

the boost documentation says it's saver.

 

OrbWeaver: so I'll try to change The ShaderTemplate.

 

edit: I uploaded the new versions

MapExpression.cpp

MapExpression.h

[greebo]

The static constructNull method appears to be just a complicated way of allocating an empty MapExpression object (named constructor):

static MapExpressionPtr constructNull() {
		return MapExpressionPtr(new MapExpression());
}

You can ditch that and initialise the members in ShaderTemplate directly:

_texture(new shaders::MapExpression())

It's likely that this is to circumvent Null-pointer checks somewhere else in the code, so that the assumption _texture != NULL is always true.