MapExpressions

[greebo]

the boost documentation says it's saver.

You're of course right, but I think the boost doc is referring to the fact that you can do things like this:

{
Image* img = new Image("test.jpg");  
boost::shared_ptr<Image> imgPtr(img);
delete img;
}

which will crash your code at the end of the scope, because the reference count of the shared_ptr will reach zero and it will try to delete the object (that has already been deleted).

 

That's why boost::shared_ptrs should always be initialised directly, using new:

boost::shared_ptr<Image> img(new Image("test.jpg"));

which locks the actual raw pointer away from the rest of the code, so that only the shared_ptr has control over it.

 

Regarding the return: it doesn't make a difference here, in terms of safety. It's just shorter to directly return the constructed boost::shared_ptr, without creating a copy. If you step over the return statement using a debugger, you will see what I mean.

[mohij]

That's not possible, since there is no MapExpression class, only IMapExpression, which is virtual. So that's not possible.

 

Edit: refering to your first post

[greebo]

Ah, well then it's also possible to initialise the _texture pointer with an empty shared_ptr object:

_texture(shared_ptr<MapExpression>())

which will technically contain a raw NULL pointer, so you will have to take care when dereferencing that object.

 

You can of course implement a NullMapExpression as well, it's up to you to decide whether this is convenient and worth it.

[mohij]

@greebo (second post): Ah, I know what you mean, you talk about the return optimization, right?

[greebo]

Yes. It won't make a noticeable difference, it's more or less nitpicking what I'm practicing here.

[OrbWeaver]

In all probability the unused return variable will be optimised out by the compiler anyway, but it's certainly clearer not to use it.

 

Creating a null shared_ptr is the way to go (if a null is needed for whatever reason), rather than creating an extra class just to represent null. If a member variable _texture is of class shared_ptr<IMapExpression>, it will be automatically initialised to null anyway when the class is constructed.

[mohij]

If I understand right, the textures need to be identified with a definite string. I have no idea what string should be used to identify a MapExpression, since a MapExpression can be build up of more than one token. Any ideas?

 

Edit: an idea I had is that every map expression also has a function

std::string getName();

which constructs it's name from the previous ones and adds it's own part to it. No idea if that would be ok.

[OrbWeaver]

I think we had some brief discussions about that during the Shaders plugin refactor, and I suspect that it would suffice to use an autogenerated texture name like "_diffuse_textures/material/name".

[greebo]

Yes, I think it's not a bad idea to equip each MapExpression with a getName() method. To ensure backwards "compatibility", the ImageMapExpressions (those delivering just the file from the disk) should just return the image name, whereas all the others generate a name like OrbWeaver suggested.

 

Btw: the name allows the GLTextureManager to cache the textures it loads into OpenGL. If the auto-generated name is chosen consistently, two MapExpressions with (for example) _heightmap_textures/material/name could be correctly identified as the same texture and not loaded twice.

[mohij]

The problem is, that the MapExpression can't know what material it is. The only thing it has is the Tokeniser (the Expression that produces them). So would it be fine to identify them by this expression?

[mohij]

Does this identifier need to be human readable in any way?

[greebo]

I'd say it should be a combination of the token returned by its children and the own "name" of the MapExpression. A Heightmap expression "knows" about itself, so it could attach the prefix _heightmap_ to the child name.

 

I imagine something like this:

• addnormals(textures/darkmod/bla, heightmap(textures/borg)) => _addnormals_/textures/darkmod/bla_heightmap_textures/borg
  
• heightmap(textures/darkmod/bla) => _heightmap_/textures/darkmod/bla
  

Does this identifier need to be human readable in any way?

Not really, it's just a string identifier.

[mohij]

Yeah, that's exactly what I wanted to do, that's why I asked about human readable

[mohij]

Small status report.

Yesterday evening and this evening I spent on fixing compiler errors...

atm I'm stuck on GLTexturemanager.cpp. I changed getBinding() to accept an Image& instead of a TextureConstructor. Problem is that empty Images need to be possible. And I didn't consider that non existant MapExpressions can't produce Images (not even empty ones ;-). So I guess I will have to rewrite the getBinding() to accept a MapExpression instead of an Image. Hopefully it will work afterwards.

[greebo]

Yes, taking an IMapExpressionPtr as argument is better. Don't hesitate to post if you're getting really weird compiler errors. Most of them are easy to fix or to search for in google (and you're learning quite a bit from googling), but some of them are really hard to find, so it might save you some headaches.

 

(I once forgot to close a namespace bracket in a header file and consequently got weird errors in the "parent" file - took me a bit to find that one.)

 

Regarding the empty Image object: That's one thing I like about pointers over references: pointers can be NULL, whereas you can't have empty references. The solution would be to implement some exception handling, which is mostly fine, but in some situations I find this cumbersome and prefer to have a pointer-like object.

 

Anyway, passing the IMapExpressionPtr should be fine here. Keep it up.

[mohij]

Small question:

Is this possible?

TexturePtr GLTextureManager::getBinding(IMapExpressionPtr mapExpr, const std::string& textureKey = mapExpr.getIdentifier()) {

 

Edit: No, doesn't work, solved it with an ugly if construct.

[greebo]

I don't know the surroundings of that, but I guess it's probably enough to pass the IMapExpressionPtr without the textureKey argument here, as this can be retrieved from the MapExpression anyway?

[mohij]

Not all the time, since the key is different sometimes (I think).

Different problem: it is not allowed to include from the plugins dir, right?

But when modifying getBinding to accept MapExpressions I need to modify the base class IGLTextureManager too. But to do that I would need to include the .h file from the plugins dir.

 

Edit: btw. getBinding has 3 different ways to be called on my local copy:

TexturePtr getBinding(MapExpressionPtr mapExp, const std::string& textureKey = NULL);
TexturePtr getBinding(const std::string& filename);

[mohij]

OK, you are right, after a bit more inspection it seems it is in fact only called with the same key. I will drop that way of calling getBinding.

[mohij]

Completely offtopic question. While compiling I get these warnings (after every file). Once I tried to fix them, but I didn't understand much. Is that important enought for a bug?

radiant/winding.h: In function 'void Winding_testSelect(Winding&, SelectionTest&, SelectionIntersection&)':
radiant/winding.h:373: warning: dereferencing type-punned pointer will break strict-aliasing rules
libs/math/Vector4.h: In member function 'const BasicVector3<T>& BasicVector4<Element>::getVector3() const [with Element = double]':
libs/math/matrix.h:439:   instantiated from here
libs/math/Vector4.h:291: warning: dereferencing type-punned pointer will break strict-aliasing rules
libs/math/Vector4.h: In member function 'BasicVector3<T>& BasicVector4<Element>::getVector3() [with Element = double]':
libs/transformlib.h:71:   instantiated from here
libs/math/Vector4.h:287: warning: dereferencing type-punned pointer will break strict-aliasing rules

[mohij]

YEEEEEEEEEHAAAAAAAAAW!! First successful compile!!

It segfaults, but anyways. *happy*

[greebo]

Congrats!

 

I got that compiler warning in Linux as well, but I haven't really looked into it. I'll do a quick search about it.

[greebo]

The compiler warning is about this:

BasicVector3<Element>& getVector3() {
return *reinterpret_cast<BasicVector3<Element>*>(_v);
}

This casts the double[4] array onto a three element vector (using brute force), relying on the assumption that the memory layout of the first three doubles will match a Vector3. Creating the pointer is valid (I think), but dereferencing is the thing that gcc dislikes.

 

We can either disable that warning by adding -fno-strict-aliasing or fix it, although I don't know if that's easy.

[mohij]

Atm my Radiant aborts, because

GLTextureManager::getBinding(MapExpressionPtr mapExp)

gets an empty MapExpressionPtr.

The old version of the MapExpressions had a function which, according to comment, evaluates to an empty texture, so getBinding worked.

 

If I understood the problem right, I have two possibilities, either introduce empty MapExpressions also and roll it up the way it was in the old code, or modify getBinding to check for empty MapExpressions and then return the empty texture. The second way has the disadvantage that any error checking is bypassed. So what'd you say?

[greebo]

It's a very common case for DarkRadiant that shaders are missing (because the map author moved them, misspelled the shader name, defined an invalid shader and so on). Do I understand it right that this is because of this?

 

Generally, I'd add a safety check to GLTextureManager::getBinding(), because passing an empty MapExpresionPtr could always happen in theory, and a crash of the application is not acceptable in any case. If the code is designed such that passing empty pointers are virtually impossible, you could add an assert:

GLTextureManager::getBinding(IMapExpressionPtr mapExpr) {
  assert(mapExpr);
  ...
}

which will throw a runtime error as soon as an empty pointer is encountered, along with more useful information about where this happened.

 

The delivery of the placeholder textures ("shader image missing", "shader not found") is currently handled by the GLTextureManager as well, so it's not a problem if you move that check into getBinding(). However, if you're able to implement a better safety check outside of this method, that's of course fine too.