MapExpressions

[mohij 0]

The problem is that I changed getBinding to take MapExpressions, so I'm forced to use MapExpressions all the time, even if I just need to realize a path which has nothing to do with a MapExpression.

 

I think even better would be to change the GLTextureManager to take such a "bool useFileLoader" instead of a MapExpression. Logicaly that makes more sense too.

 

Edit: or even better change the

GLTextureManager::getBinding(const std::string& filename

to default to using the FileLoader and just let the caller create the MapExpression.

[greebo 86]

Agreed, it's better to change the GLTextureManager interface here. If I recall correctly, the default shaders are called directly from CShader as fallback, right?

[mohij 0]

Ok, fixed this.

Link: MapExpression.zip

[greebo 86]

Ok, I'm about to commit the changes. I introduced the module names as optional argument to GLTextureManager::getBinding() and adapted the abstract base class in ishaders.h. This way the method is more versatile when it comes to load file formats other than BMP, which may be desired. The default is still BMP, but can be changed to be GDK or TGA or anything.

 

As usual I have some feedback : If you're introducing new public methods to classes, like the overloaded getBinding() method, be sure to add some comments about what this method does and what the parameters are supposed to mean. That's not your fault in this case (I think I forgot to comment it myself), so don't take this as criticism. With a codebase as large as DarkRadiant's it is very likely that you forget what your own code does if you don't work on it for half a year or so - and there's three of us coders right now, which makes comments even more important.

 

The lack of comments was/is one of the greatest weaknesses of the old GtkRadiant codebase, there were nearly no comments at all and it was chaotic - a state which we are slowly getting away from.

 

So if you encounter undocumented things and think you understand it, drop some comments there, this is invaluable.

[mohij 0]

I noticed a bug: in GLTextureManager.h line 85

virtual TexturePtr GLTextureManager::getBinding(const std::string& fullPath, const std::string& moduleNames) = 0;

the GLTextureManager:: shouldn't be there.

 

Small question, is it intended, that you use two different descriptors for the second argument? In ishaders.h and GLTextureManager.h you use "moduleNames" and in GLTextureManager.cpp "fileExt".

 

What are you working on next?

I will continue with the AddNormalsExpression. Some information on what this function (and later on the others too) should actually do would be helpful. The alternative would be trying some implementations and see which one mimics Doom3s implementation best, but that doesn't sound fun...

 

Edit: and about the critics, I am very grateful for them, since they really help learning, and that's what I want to do ;-)

[greebo 86]

I noticed a bug: in GLTextureManager.h line 85

virtual TexturePtr GLTextureManager::getBinding(const std::string& fullPath, const std::string& moduleNames) = 0;

the GLTextureManager:: shouldn't be there.

Yet again! I seem to forget to remove this quite often (my gcc 3 doesn't complain about it, sorry)... commit is underway.

 

Small question, is it intended, that you use two different descriptors for the second argument? In ishaders.h and GLTextureManager.h you use "moduleNames" and in GLTextureManager.cpp "fileExt".

No, that's not intentional. I renamed it in all three files but I think I hit undo or something and forgot to redo that one. Thanks for spotting that.

 

I will continue with the AddNormalsExpression. Some information on what this function (and later on the others too) should actually do would be helpful. The alternative would be trying some implementations and see which one mimics Doom3s implementation best, but that doesn't sound fun...

My (uneducated) guess would be that addnormals() adds the two vectors and renormalises them, but this is just a guess. Some research will definitely be in place here, I reckon.

[OrbWeaver 663]

My (uneducated) guess would be that addnormals() adds the two vectors and renormalises them, but this is just a guess. Some research will definitely be in place here, I reckon.

 

That's precisely what it does AFAIK. The best thing to do would be to parse the image data into double-precision vectors, add them mathematically, renormalise and then convert back into the <R, G, B> values for a normal map.

[mohij 0]

My first try would have been to write a normalize function, as I will probably need that one more often. And then simply iterate through the image (I wouldn't even need to differentiate between RGB) add the values together and divide by 2. After that a call to the normalize function would do the trick.

That is much simpler and the result would be the same, right?

 

I'm not 100% sure, normalize means trimming a vertor to a length of 1, right?

Edit: Wikipedia helped.

[OrbWeaver 663]

I'm pretty sure there is already a normalise function available for Vector3, or at least if there isn't it can easily be performed by Vector 3 newVect = oldVec * (1 / oldVec.length());. Adding the values together and dividing by 2 probably does the same thing though, and you might indeed find that it was faster.

[greebo 86]

There is the Vector3::getNormalised() method, which returns the normalised vector, so this would do the trick:

   Vector3 result = (imgAnormal + imgBnormal).getNormalised();

 

edit: Although I'm not sure whether all vectors of a normalmap are normalised in the first place, but they should, shouldn't they?

[mohij 0]

I completed the AddNormals, but the results don't look the way they should.

Any ideas what could cause that?

Solved it.

[mohij 0]

The changes:

MapExpression.zip

[mohij 0]

I just finished the smoothnormals Expression. But it seems there is no way I can test it, since no material in doom3 seems to use it 8-|. Could someone help me out with a material file?

[mohij 0]

Same goes with add and scale.

 

Another problem:

iddevnet states that the scale expression scales the RGBA channels, which would be very easy to implement.

modwiki states that it in fact scales the image, which is quite hard to do.

[OrbWeaver 663]

A scale map expression only scales the RGBA values. The "scale" parameter used as part of a material stage does change the size, but this is not related to map expressions.

[mohij 0]

Do you have any material files using those MapExpressions, or at least some information what the values exactly do?

[OrbWeaver 663]

As far as I know, it is simply a multiplication of the first map parameter by the second float parameter, which goes from 0.0 (full black) to 1.0 (no change). Values above 1.0 are not accepted AFAIK.

 

I'm guessing the optional parameters allow you to scale R, G, B, A separately, but I have never seen this used.

[mohij 0]

Well then, latest progress:

MapExpression.zip

 

implemented are: heightmap, addnormals, smoothnormals, add and scale.

The rest should be relatively easy.

 

Since there are no material files I can try the add and scale on, I can't say if they work.

[greebo 86]

Good progress, this is coming along nicely!

 

I committed your current state to the mapexpressions branch. I tested it with this shader:

textures/darkmod/stone/trim/ornament_010
{
stone

qer_editorimage	textures/darkmod/stone/trim/ornament_010_ed
diffusemap	textures/darkmod/stone/floor/rough_005_d
bumpmap		addnormals(textures/darkmod/stone/trim/ornament_010_local, textures/darkmod/stone/trim/grave_005_roughness_local)

which fails the assertion:

Assertion failed: width == imgTwo->getWidth(), file plugins\shaders\MapExpression.cpp, line 99

because the dimensions of the two images are not matching. I think combining two non-matching images is valid, so we probably need to implement variable sized stepping for one of the two images.

[greebo 86]

I have three more comments on the code:

• I added the release() calls to delete the source images after they've been used to create the result image, to avoid memory leaks.
  
• This (*i). is the same as i->, but the latter is shorter (very minor, this is used throughout the GtkRadiant codebase).
  
• The standard Vector3 is the defined as BasicVector3, so I replaced the BasicVector3 with Vector3, which is shorter. So, unless you need non-double vectors like BasicVector3 or anything else, you probably just want to use Vector3.
  

[mohij 0]

The scale problem could be solved in two ways. Either scale one image to the size of the other (probably the higher resolution image to the lower) or don't scale the images and use the getPixel function, that won't scale the images, but repeat the small image. Both seem logical, so no idea which one is correct.

 

Scaling the image would require a scale function (perhaps there already is one in there I don't know of?), writing my own would be quite some work, an alternative would be a hack to divide the different widths and heights and step through the bigger image with a different offset, but that's no nice solution.

[greebo 86]

I don't know which method is the correct one, that should probably be tested with two normalmaps that are easy to recognise. This would definitely be nice to know at any rate.

 

There is a scaling method in TextureManipulator, which is a local helper class of the GLTextureManager. We can either turn that TextureManipulator into a singleton class with a global accessor method or add a GLTextureManager::getManipulator() accessor, which returns the reference to the member instance.

[mohij 0]

I did a small test, and it seems to scale the second parameter to the resolution of the first one.

 

I also noticed that the resolutions of the images are not responsible for the size of the final result. I guess the diffuse map resolution does set the size finaly. I will test and report results. (so in the end all images in a material file will have the same resolution, I'll just have to find out which one sets this final resolution).

[greebo 86]

I'm pretty sure that the shader images (bumpmaps, speculars, diffuse) can have different dimensions, but I guess it makes sense that the second image gets scaled to match the first when using addnormals().

[OrbWeaver 663]

You don't need to worry at all about the relative sizes of different stages (diffuse, bump etc), that is all taken care of by the texture alignment code. The scope of a map expression extends no further than producing an image for a single stage.