MapExpressions

[mohij]

The MapExpressions are only implemented on a very basic level at the moment. They should be reimplemented in a better way.

 

At the moment I need to understand how they are used (or should be used) and how the new design should look.

 

OrbWeaver said something about a hierarchy of polymorphic objects and an IMapExpression interface.

The IMapExpression interface sounds like a module, I'm not sure whether a completely new module is appropriate for such a special thing as MapExpressions.

Polymorphic objects sound good, but I'm not sure how the hierarchy should be done, the current implementation seems a bit trival, perhaps there is a better solution.

 

Is the functionality of the MapExpressions (addnormals, hightmap, etc.) implemented somewhere already, or do I have to write those from scratch too?

 

So that's where I currently am, tell me what you think.

[greebo]

OrbWeaver knows best about the current MapExpressions hierarchy, I know more about the ShaderSystem itself, which is surrounding all that stuff.

 

As for the functionality: the algorithm for addnormals() is nowhere implemented so far (this will need some serious thinking and/or googling I guess), but I have saved a heightMapCreator code snippet in the shaders module, which looks functional (haven't tested it).

[OrbWeaver]

OrbWeaver said something about a hierarchy of polymorphic objects and an IMapExpression interface.

The IMapExpression interface sounds like a module, I'm not sure whether a completely new module is appropriate for such a special thing as MapExpressions.

Polymorphic objects sound good, but I'm not sure how the hierarchy should be done, the current implementation seems a bit trival, perhaps there is a better solution.

 

All modules have an interface, but not all interfaces are modules. I mean an interface similar to the following (within the shaders module):

 

class IMapExpression {
public :
virtual ImageConstructor& getImage()  = 0;
}

 

Where ImageConstructor is the lazy-evaluating image construction object that Greebo implemented in the shaders module.

 

Individual map expression types would then implement this interface, so that you would have a HeightMapExpression, an AddNormalsMapExpression and so on. When the getImage method was called, the map expression subclass would recursively evaluate any "child" IMapExpressions it contained, which in most cases would be a simple texture name but could also include any number of recursive calls to addnormals() etc, and then perform its own class-specific image processing on the results in order to return its own "flattened" image.

 

How best to construct the parse tree requires some consideration too, the process is based around a stream of tokens retrieved from a parser::DefTokeniser object. I am not immediately sure whether each MapExpression subclass should construct itself from a token stream, or whether there should be a separate service class which reads the token stream and constructs the relevant MapExpressions explicitly.

[mohij]

I think an extra "parsing class" that creates the MapExpressions would be better. I think it would be easier to handle as things are seperated.

[mohij]

After thinking about a service class, I am not sure anymore whether it would be easier with an extra service class.

This extra service class would have to seperate between the different Tokens and work differently for different MapExpressions (as the different MapExpressions need different input).

So I think implementing a service class that differenciates between the different MapExpressions and parses them the input they need would end in one big service class that does all the work and very small MapExpression classes that could as well be only functions. (the polymorphism wouldn't get used).

 

MapExpressions that call each other are perhaps still better.

[OrbWeaver]

Well you're going to need some kind of factory class obviously, in order to create the correct MapExpression class from a given token, but you are correct that the MapExpressions will need to parse their own token stream, otherwise as you say the logic will all be in the service class and the MapExpressions won't really do anything.

 

You could have a static method in the base class, e.g.

 

class IMapExpression {
public:
boost::shared_ptr<IMapExpression> createForToken(const std::string& token);
}

 

which would look up the string token in a static std::map and return a new instance of the correct MapExpression. The parsing code in each MapExpression would then need to detect that there was a recursive expression inside it, and call this function to create an instance of the required class.

 

This means that for parsing the expression

 

map addnormals(textures/blah, heightmap(textures/bleh))

 

the sequence would be

 

1. Shaders code detects the "map" keyword which introduces a map expression, then parses the next token.

2. Token is passed to the static factory method to create the required MapExpression. It looks in its map and constructs an AddNormalsExpression which is returned to the shaders code.

3. Shaders code passes the tokeniser to AddNormalsExpression.

4. AddNormals expression reads the "(".

5. AddNormals expression reads the next token, and detects it is a texture. This is added as the first child for later processing (you might want an ImageMapExpression that is another subclass of IMapExpression representing a single texture name).

6. AddNormalsExpression reads the ",".

7. The next token is heightmap, which requires a new MapExpression. The factory method is called once again, and returns a HeightMapExpression.

8. AddNormalsExpression passes the current tokeniser to the new HeightMapExpression.

9. HeightMapExpression parses the bracket, texture name and bracket. It adds the texture as its single permitted child.

10. The construction of HeightMapExpression is complete. Control returns to the AddNormalsExpression which parses the final ")".

11. Construction of AddNormalsExpression is complete. Control returns to the shaders module, which now has a complete parse tree representing the map expression in its entirety.

12. Shaders module continues where it left off, reading tokens from the Tokeniser.

[mohij]

With such a createForToken function I wouldn't even need an extra factory class. The method would simply be called for the base class by the Shader code, that's cool.

And I think that this single function should already suffice as the public interface for the class.

I will try/start to implement it tomorrow, I guess the next big task will be the MapExpressions themselves.

[mohij]

Would a

boost::shared_ptr<IMapExpression> createForToken(parser::DefTokeniser& tok)

external friend function also be acceptable instead of another class? I got stuck while trying to use this function as a member of class IMapExpression, because you can't instanciate a class that is has vitual, but need to, because the createForToken is member of it. So simplest solution would be to make it external, but friend. Is that acceptable?

[OrbWeaver]

The method needs to be static, so that it is not associated with a specific class instance.

[mohij]

I want to create it as a global function. It shouldn't associate to some class, just because I made it friend of one, should it?

[OrbWeaver]

There is no need for a friend function, this is the wrong idiom. A public static function is equivalent to a C function at file scope, in that it can be called without any class instance, e.g.

 

IMapExpressionPtr expr = IMapExpression::createForToken("heightmap");

 

It doesn't have to be associated with a class as far as the compiler is concerned, but this is a stylistic device used for tidiness. There is going to need to be a static map associating IMapExpression subclasses with string tokens, and this should be a member of the same class.

 

Essentially, the IMapExpression interface is also functioning as its own factory class.

[mohij]

Hey, that is what I tried to do before, but then the compiler told me that it is plainly impossible to call functions from non instantiated classes, now I know better . Cool, learned another thing, thanks. This way it is of course nicer.

[mohij]

I'm away over the weekend, so sadly I won't be able to continue on this before monday. I hope this won't break any timelines.

[OrbWeaver]

Ha, if there were deadlines on this project I would have had to fire myself several times over for low productivity.

[mohij]

It's quite relieving to hear that, I already felt bad a little bit

So till monday then.

[Crispy]

I, too, would have been unceremoniously thrown onto the street a long long time ago if we had strict deadlines to stick to. Get things done as quickly as you can by all means, but don't stress out if you don't have time.

[Nyarlathotep]

I'm afraid I'm even worse. I had to drop off the forums for three weeks due to finals and vacationing in Sydney. I've yet to do anything since I've gotten back (apart from playing Twilight Princess). Deadlines are my kryptonite.

[mohij]

I'm back.

[greebo]

Good to hear that.

 

I didn't follow the discussion above very closely, but maybe a small sketch of the system to be implemented might prove useful here? We did that back when we redesigned the ShaderSystem, and it was good to have some plans around - also for later reference and/or the wiki.

 

Not a must of course, just a suggestion.

[mohij]

Well, what I have since now is:

 

Base class:

 class IMapExpression {
   public:
	   static boost::shared_ptr<IMapExpression> createForToken(parser::DefTokeniser& tok);
	   virtual TextureConstructor& getImage() = 0;
   };

 

specific Map Expressions then look this way:

class HeightMapExpression : IMapExpression {
	   shared_ptr<IMapExpression> argOne;
	   float argTwo;
   public:
	   HeightMapExpression (parser::DefTokeniser& tok);
	   TextureConstructor& getImage();
   };

 

implementation for createForToken:

boost::shared_ptr<IMapExpression> IMapExpression::createForToken(parser::DefTokeniser& tok) {

	 // Switch on the first keyword, to determine what kind of expression this
	 // is.
	 std::string token = boost::algorithm::to_lower_copy(token.nextToken());

	 if (token == "heightmap") {
		 return shared_ptr<IMapExpression> p(new HeightMapExpression (tok));
	 }
	 else if (token == "addnormals") {
		 return shared_ptr<IMapExpression> p(new AddNormalsExpression (tok));
	 } ...

 

I'm not entirely sure where the images should be saved, inside the structures? Also not sure when they should be processed. Either when creating the class or when getImage is called. So, comments please

[mohij]

Forgot, there is also an "ImageExpression" which just returns an image.

 

Edit: is there a way to upload files (.h and .cpp)? That would make things easier, as I could just paste the files I've written so far.

[greebo]

Yes, we have an FTP server, please read here how to access it: http://forums.thedarkmod.com/index.php?showtopic=609

 

Do you have some material files for testing? Otherwise I could provide a few to facilitate testing.

 

(Looks good so far, the devil is probably in the detail as usual, but I get the idea.)

[mohij]

What directory would be appropriate? There is a temp and a DarkRadiant dir.

I just have the standard doom3 files, no idea if they suffice.

[greebo]

The DarkRadiant directory will be good enough for this, it's just the snapshot ZIP lying around there, so feel free to drop it there.

 

The standard Doom3 files can be enough I think, just do a grep for addnormals() or heightmap in the material files, I think they are in pak001.pk4 or pak002.pk4.

[mohij]

So here is my current progress:

MapExpressionnew.cpp

MapExpressionnew.h

 

I just looked at the surrounding files and I think I will have to change some other files too to make them work with the new interface, right? Then this should be done first, so I can test the code I write.

 

 

Should the processed images be saved at all (or just created every time the Shadersystem requests one) and if yes, where?

In the Constructor: all MapExpressions would be evaluated, independent of their usage

getImage could also be defined in the base class

in getImage: either the MapExpression is recreated everytime it is asked for, or saved (either in the class or pushed to the texture manager)

 

I think in getImage would be better. Different from the files above, I think it would be better to save the results instead of the arguments.

Edit: I changed MapExpressionnew.h to reflect this.