MapExpressions

[mohij]

But it would be good to know anyways.

 

What I know for sure now is that in map expressions the second image is scaled to the size of the first.

 

That different dimensions are possible is obvious too, but what I'd find interesting to know is what sets the final size of the texture in game. (whether a texture is "two feet" or "20 feet" high.)

 

It seems the resolution has no influence on that. Could it be that by default all textures are the same size?

[mohij]

Now that it's obvious that we'll need this scale function/class, could you (greebo) please explain what exactly you meant by turning the TextureManipulator into a singleton? I don't really understand how that makes sense (singleton = only one object of that type, right?).

[greebo]

That different dimensions are possible is obvious too, but what I'd find interesting to know is what sets the final size of the texture in game. (whether a texture is "two feet" or "20 feet" high.)

 

It seems the resolution has no influence on that. Could it be that by default all textures are the same size?

When texturing surfaces, you don't address them using pixel coordinates, you're using U/V coordinates instead.

 

Imagine a 512x256 pixel texture. A U/V coordinate pair of refers to the "upper left" point in the imagemap, whereas the lower left corner of the image has the U/V coordinates . This is how all the models, patches (and technically every vertex) are textured, and this is independent of the actual size of the texture. If you decide to increase the resolution of the texture, you just do it - the texture coordinates don't change, but the resolution when you look at the model is of course improved.

 

@Singleton: Yes, singleton means that there is only one instance of this class. This makes sense for classes that are expensive in terms of memory, initialisation or where several instances would cause conflicts.

 

Basically, a singleton class is defined like every other class. To turn it into a singleton, you don't instantiate it by making it a member of another class or by calling new, but you define an accessor method:

 

class MyClass
{
public:
  // Constructor and everything else can go here

 void printHello() {
 std::cout << "Hello.\n";
 }

 // Accessor method containing the singleton instance
 static MyClass& Instance() {
  static MyClass _instance;
  return _instance;
 }
};

The static member can be called like this:

MyClass::Instance().printHello();

As soon as Instance() is called for the first time, the static singleton _instance is instantiated (the constructor gets called) and a reference to it is returned. The next time you call Instance(), the class is already instantiated, so there is no need to call the constructor once again.

 

Every access to this class should be performed using the accessor method, as it's technically still valid to create other instances of MyClass.

 

An example of such a singleton class is the GameManager in radiant/settings/GameManager.h (there are many other examples).

 

Feel free to ask more questions, I don't know if I'm good at explaining these things.

[mohij]

For a game manager that makes sense (since there can only be one), but with a texture manipulator that is a class of which logically any number could exist (with threads even at the same time), that seems strange. To me it sounds quite strange to make classes singletons which logically aren't, just as a speed hack. Or did I misunderstand that?

 

Following that thought it means that there is no actual data saved in this class (since the same class can be used in different cases), so the class actually just wraps up functions.

Wouldn't it then be better to just provide the functions directly? (this doesn't sound object oriented, but why make a class out of something which is really just functions?)

 

Please don't feel offended, it just doesn't seem right to me.

[greebo]

No problem, no offense taken.

 

The TextureManipulator does indeed maintain its own member variables, it's not just a container for the algorithm. It keeps its own gamma table and some member variables for the texture quality that should be used to process the image. These members correspond to some settings in the XMLRegistry and get initialised during construction. That's why the class is also deriving from RegistryKeyObserver, so that it gets notified as soon as the configuration settings are changed, which gives it a chance to react.

 

While it would be possible to have several instances of this TextureManipulator, it is a bit wasteful to maintain more than one of them. Each instance would keep its own gamma table and would register itself as RegistryKeyObserver on construction and de-register at destruction, which creates unnecessary overhead imo.

 

Admittedly, this is not a performance-critical example and having multiple TextureManipulator classes won't do much harm, but I find it cleaner to have just one central service class for this.

[mohij]

Ok, now I understand, thanks for those explanations. I'll try to change this to a singleton.

[Crispy]

Every access to this class should be performed using the accessor method, as it's technically still valid to create other instances of MyClass.

Though note that if you want to explicitly forbid creating other instances of MyClass, you can make all the constructors private or protected (protected is better IMO).

[mohij]

Okay, new version with scale support is up.

MapExpression.zip

 

and btw. cool to hear about 0.9.1 ;-)

[greebo]

Thanks for the upload, I'll compile right now.

 

One thing I noticed in AddnormalsMapExpression::getImage(): You have included a proper release call for all the objects you create, but there is a trap in there:

	// resized version of imgTwo
Image* imgTwoRes;

if (width != imgTwo->getWidth() || height != imgTwo->getHeight()) {
imgTwoRes = new RGBAImage(width, height);
TextureManipulator::instance().resampleTexture( imgTwo->getRGBAPixels(), imgTwo->getWidth(), imgTwo->getHeight(), imgTwoRes->getRGBAPixels(), width, height, 4);
}
else {
imgTwoRes = imgTwo;
}

Later on you call

	imgOne->release();
imgTwo->release();
imgTwoRes->release();

which will probably crash if imgTwoRes == imgTwo due to the double release. You can't delete an object from the heap twice, the app will most certainly crash here, so the algorithm will have to be tuned to avoid double-deletions.

 

This is of course just one more argument to use boost::shared_ptr pointers instead, which will take care of this themselves, so the algorithm may be simplified in the future.

 

I'll look at the code in more detail now.

[mohij]

New version with all expressions completed and smooth normals refactored to use Vector3

MapExpression.cpp

MapExpression.h

 

The SmoothNormals doesn't work correct and I'm quite stuck, because I really can't explain the faulty result...

 

Should look like this:

[greebo]

I changed the algorithm to take care of the aformentioned issues and committed them to SVN, so you can take a look at it.

 

I'll have a look at your new files now.

[greebo]

I think I found the problem, although it's still not entirely working (I get some repeatedness into the texture, I haven't looked where this is coming from):

 

You have this loop:

 // iterate through the pixels
for( unsigned int x = 0; x < height; x++ ) {
for( unsigned int y = 0; y < width; y++ ) {

but x should be iterating through the columns and y through the rows, hence it should be:

 // iterate through the pixels
for( unsigned int y = 0; y < height; y++ ) {
for( unsigned int x = 0; x < width; x++ ) {

This itself wouldn't have been a problem, but in combination with the getPixel() helper (which translates x to the columns and y to the rows) there is a mapping going on between the images.

 

Currently, I'm getting this for a smoothnormal (left = ordinary normalmap, right = smoothnormals):

[greebo]

I also found that normalmap vectors aren't normalised (as per having a length of 1 - the normal pointing 90° from the face has for instance, which is not normalised). This is why in the above picture, the smoothed normal visible on the right looks darker, because weaker lighting information gets baked into the resulting image.

 

I replaced the normalisation with an ordinary mean vector (sum divided by kernel size) and the amount of lighting is correct now, but I still get the repeating images as in the above screenshot, which I have to figure out.

 

It is most likely that the addnormals() algorithm is affected as well, we might want to change that too.

[greebo]

Ok, found the error. The getPixels() routine is expecting the pixels to point to the beginning of the image buffer, but the pointer was incremented each turn (probably a copy&paste mistake). I wonder why this thing didn't crash but it seems that getPixels did a good job on clamping the value.

 

I think there was also a problem with using unsigned int as loop variable type, as the (x + i->dx) values can be negative as well, maybe this was also ironed out by the getPixels modulo operations.

 

However, this is the result (soon in your latest SVN branch):

 

Btw: we have just reached revision 2000. Woot!

[mohij]

greebo, you simply rock. Wow.

[mohij]

Shall I beautify the other functions the way you did? I guess that makes sense.

[mohij]

I cleaned the MapExpression.cpp up, always have y first in for clauses, removing a lot of whitespaces and bring the scaleexpression to a usable state by inserting some error checking.

MapExpression.cpp

[mohij]

I cleaned the MapExpression.cpp up, always have y first in for clauses, removing a lot of whitespaces and bring the scaleexpression to a usable state by inserting some error checking.

MapExpression.cpp

 

Edit: I tied out all expressions, and all seem to work. I guess I'm finished... or did I miss anything?

 

oh, and btw. next week I won't be able to work on DR, since I will have my oral exams monday in a week and that will probably consume all my time :-(

[greebo]

greebo, you simply rock. Wow.

Heh, thanks for that.

 

I cleaned the MapExpression.cpp up, always have y first in for clauses, removing a lot of whitespaces and bring the scaleexpression to a usable state by inserting some error checking.

I'll upload your changes this evening; the cleanup definitely makes sense for the other stuff as well. It probably makes sense to remove any duplicates in the code (I guess the resampling of images can a bit simplified, and some algorithms can be moved into a header file like the HeightmapCreator.h, but nothing critical.)

 

As soon as this is done, we can merge in these changes into the trunk, after some tests.

 

oh, and btw. next week I won't be able to work on DR, since I will have my oral exams monday in a week and that will probably consume all my time :-(

No problem, I won't be able to do much myself. I'll have to get my diploma stuff finished next week so that I can register for my final exam.

[mohij]

One small bug in GLTexturemanager.cpp, at the very end there is a } missing.

[Crispy]

which will probably crash if imgTwoRes == imgTwo due to the double release. You can't delete an object from the heap twice, the app will most certainly crash here, so the algorithm will have to be tuned to avoid double-deletions.

You can however call "delete NULL;" safely. So in some cases it's possible to avoid double-release crashes by setting the pointer in question to NULL after deleting it. I don't know what the release() method is doing exactly though, so I don't know if that technique can be used here or not.

 

(You could of course explicitly check if imgTwoRes==imgTwo in this situation, but it would be nice if release() handled it - and if the Image class is deleting an internal pointer, then it sounds like you could just NULL out the internal pointer after deleting it to avoid the crash.)

[greebo]

One small bug in GLTexturemanager.cpp, at the very end there is a } missing.

Really? My version compiles just fine. Is this supposed to be a closing namespace bracket?

 

You can however call "delete NULL;" safely. So in some cases it's possible to avoid double-release crashes by setting the pointer in question to NULL after deleting it. I don't know what the release() method is doing exactly though, so I don't know if that technique can be used here or not.

 

(You could of course explicitly check if imgTwoRes==imgTwo in this situation, but it would be nice if release() handled it - and if the Image class is deleting an internal pointer, then it sounds like you could just NULL out the internal pointer after deleting it to avoid the crash.)

Thanks, didn't know about delete NULL. The pointer is definitely not reset to NULL after deletion, so it probably would have been crashing. We already solved it another way though.

[mohij]

@greebo: I just looked in viewvc and the closing brace is there. It seems that I somehow deleted it in my local copy, sorry for that.

[greebo]

I've merged in the changes from the MapExpressions branch into the trunk, as everything seems to work fine.

 

What is left to do in this section? Do we have proper support for the various shader stages?

[mohij]

I'm looking in here the first time since monday and I see those good news, cool. :-)

 

I rechecked every of those MapExpessions, and all seem to work as they do in Doom3, except the scale expression, which always produces a black texture in Doom3. But as long as nobody knows how this is meant to be used there is probably nothing we can do.

Well the only other thing that comes to my mind is this shared_ptr refactoring you talked about. And I don't really know what you mean with shader stages...

 

On Tuesday next week I will have finished my exams and can continue on something (whatsoever it might be).