Jump to content
The Dark Mod Forums

Recommended Posts

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.

Link to post
Share on other sites
  • Replies 314
  • Created
  • Last Reply

Top Posters In This Topic

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?

Link to post
Share on other sites

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.

Link to post
Share on other sites

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 ;-)

Link to post
Share on other sites
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.

Link to post
Share on other sites
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.

Link to post
Share on other sites

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.

Link to post
Share on other sites

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.

Link to post
Share on other sites

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?

Link to post
Share on other sites

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.

Link to post
Share on other sites

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.

Link to post
Share on other sites

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.

Link to post
Share on other sites

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.

Link to post
Share on other sites

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.

Link to post
Share on other sites

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).

Link to post
Share on other sites

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().

Link to post
Share on other sites

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.

Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


×
×
  • Create New...