Jump to content
The Dark Mod Forums

MapExpressions


Recommended Posts

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?

Link to comment
Share on other sites

  • Replies 314
  • Created
  • Last Reply

Top Posters In This Topic

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

Link to comment
Share on other sites

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

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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

My games | Public Service Announcement: TDM is not set in the Thief universe. The city in which it takes place is not the City from Thief. The player character is not called Garrett. Any person who contradicts these facts will be subjected to disapproving stares.
Link to comment
Share on other sites

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

Link to comment
Share on other sites

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

t757903_smoothnormals1.jpg

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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

t758152_smoothnormals2.jpg

 

Btw: we have just reached revision 2000. Woot!

Link to comment
Share on other sites

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 :-(

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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

My games | Public Service Announcement: TDM is not set in the Thief universe. The city in which it takes place is not the City from Thief. The player character is not called Garrett. Any person who contradicts these facts will be subjected to disapproving stares.
Link to comment
Share on other sites

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

Link to comment
Share on other sites

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?

Link to comment
Share on other sites

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

Link to comment
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.


  • Recent Status Updates

    • Petike the Taffer  »  DeTeEff

      I've updated the articles for your FMs and your author category at the wiki. Your newer nickname (DeTeEff) now comes first, and the one in parentheses is your older nickname (Fieldmedic). Just to avoid confusing people who played your FMs years ago and remember your older nickname. I've added a wiki article for your latest FM, Who Watches the Watcher?, as part of my current updating efforts. Unless I overlooked something, you have five different FMs so far.
      · 0 replies
    • Petike the Taffer

      I've finally managed to log in to The Dark Mod Wiki. I'm back in the saddle and before the holidays start in full, I'll be adding a few new FM articles and doing other updates. Written in Stone is already done.
      · 4 replies
    • nbohr1more

      TDM 15th Anniversary Contest is now active! Please declare your participation: https://forums.thedarkmod.com/index.php?/topic/22413-the-dark-mod-15th-anniversary-contest-entry-thread/
       
      · 0 replies
    • JackFarmer

      @TheUnbeholden
      You cannot receive PMs. Could you please be so kind and check your mailbox if it is full (or maybe you switched off the function)?
      · 1 reply
    • OrbWeaver

      I like the new frob highlight but it would nice if it was less "flickery" while moving over objects (especially barred metal doors).
      · 4 replies
×
×
  • Create New...