MapExpressions

[mohij]

Ok, I'm freaking out right now!!! I implemented it with the second approach (and there is some obvious flaw in there, but I don't entirely understand the problem yet) and radiant STARTS!!!!

 

Edit: but segfaults on creation of a brush :-(

[greebo]

Are the MapExpressions working? Can you see any textures?

 

edit: where exactly does the crash happen?

[mohij]

Crashes on creation of brush, so no idea if MapExpressions work ;-)

 

The first (old) crash/assertion happend in getBinding where the GLTextureManager got an empty MapExpression and asserted. If I understand it right it shouldn't assert on empty MapExpressions, but produce the getShaderNotFound(). (if I interpreted it right the old getBinding did exactly that).

 

The current crash happens in GLTextureManager::load, because the getBinding parsed it an empty image. But since it checks for empty MapExpressions I wonder how it is possible that such a MapExpression returns an empty Image.

 

Well I guess it's quite hard to understand what I talk about when you didn't read the code... If you are interested I could send you my plugins/shaders folder.

[greebo]

Yes, I guess sending the files makes sense, it's probably easier to discuss it.

 

(I wonder if creating a separate branch for you on SVN made sense. So both OrbWeaver and me could have a look at your changes. As long as you don't mess up the trunk, it should be fine. OrbWeaver, would this be ok?)

[mohij]

MapExpression.zip

here you go

[mohij]

Added another check for empty textures into getBinding, and now radiant works (with adding a brush, haven't tested anything else for now). But the brush is plain white (with standard texture).

 

Texture browser shows a lot of white textures, but some also show correctly. It seems that just the "shader not found" texture does not work.

 

Edit: In dynamic light mode the textures show no change to no dynamic light mode.

 

I will probably not be able to continue on this until tomorrow evening :-(

[greebo]

I'm compiling right now, I'll post back here in a few minutes, stay tuned.

[greebo]

Ok, seems like you're running into a NULL pointer. This is what's causing it:

Image& ImageExpression::getImage() {
DefaultConstructor d (_path);
return *d.construct();
}

The d.construct() is returning a NULL pointer and you're dereferencing it using the asterisk, as you need an Image& reference. This is actually valid, but DarkRadiant will crash as soon as you try to use the reference. This is probably one of the few ways to actually construct a "NULL-reference", so you might want to change it:

 

For now, I'd say, go with returning an Image* pointer - we can change that in another refactoring sweep to use a boost::shared_ptr instead. Add a NULL-pointer check to GLTextureManager, so that the according action can be taken (return the default shader).

 

A few other things I've found:

 

(Minor) You can treat shared_ptrs the same way as ordinary pointers, so you don't need to do this:

Image& img = mapExp.get()->getImage();

you can directly dereference the shared_ptr (it implements an -> operator):

Image& img = mapExp->getImage();

This has the "advantage" that boost will throw an exception if you try to dereference an empty shared_ptr. By calling get() you circumvent that check. Not that this will save you from a crash, but at least it gives you a hint.

 

(Major) Also, you get a "warning: control reaches end of non-void function" in getBinding(), because it's possible that the algorithm doesn't return anything (if the texture is already in the cache, for instance). This return statement:

return _textures[identifier];

Should be outside the block. If the identifier is not found, the block is executed and it is added. If it is found, it's safe to access the map with _textures[identifier], so this should always work.

 

(Very minor) I've got a weird formatting in MapExpression.cpp:

std::string AddExpression::getIdentifier() {
std::string identifier = "_add_";
identifier.append(argOne.get()->getIdentifier() + argTwo.get()->getIdentifier());
return identifier;
}

which is probably due to the mixture of tabs and spaces. Not a real problem, it's just damn hard to read on my end.

 

(Major) You've got a logical mistake in CShader which breaks the lighting rendermode:

if (!_template._bump) {
mapExp = _template._bump;
}
else {
std::cout << "Bumpmap flat image!" << std::endl;
mapExp = IMapExpression::createForString(GlobalRegistry().get("user/paths/bitmapsPath") + IMAGE_FLAT);
}

It should actually be:

if (_template._bump) {

So that the _bump MapExpression is actually used when it's not empty. Same goes for getSpecular(), the lightfalloff is ok.

[mohij]

About the NULL pointer and the "control reaches end of non-void function", I had those fixed already, even though I did it with references. I changed it to pointers now.

 

About shared pointers and .get()-> ... changed that, very good to know, thanks. If I had known that before... ;-)

 

Sorry about the formating, I use (vim)

set softtabstop=4
set noexpandtab
set shiftwidth=4

Which means an indenting of 4, but with the standard tab width of 8 (rest is filled up with spaces).

 

Logical mistake in CShader: yeah, right, got those, thanks.

 

Will upload the new version. The code looks definitely nicer, but the default texture is still white and not "shader not found".

[greebo]

Ah, ok it's probably the tab width of 8 (mine is 4 characters wide).

 

I changed the uploaded version of GLTextureManager::getBinding from this:

TexturePtr GLTextureManager::getBinding(MapExpressionPtr mapExp) {

//	check if we got an empty MapExpression
if (mapExp != NULL) {

	// Check if the texture has to be loaded
	std::string identifier = mapExp->getIdentifier();
	TextureMap::iterator i = _textures.find(identifier);
	if (i == _textures.end()) {

		// how can this command successfully work, but produce an empty image?
		Image* img = mapExp->getImage();

		// see if the MapExpression returned a valid image
		if (img != NULL) {

			// Constructor returned a valid image, now create the texture object
			_textures[identifier] = TexturePtr(new Texture(identifier));

			// Bind the texture and get the OpenGL id
			load(_textures[identifier], img);

			// We don't need the image pixel data anymore
			img->release();

			globalOutputStream() << "[shaders] Loaded texture: " << identifier.c_str() << "\n";
			return _textures[identifier];
		}
	}
}
// We got either an empty MapExpression or the MapExpression returned an empty image, return the "image missing texture"
globalErrorStream() << "[shaders] Unable to load shader texture";
return getShaderNotFound();
}

 

to this:

TexturePtr GLTextureManager::getBinding(MapExpressionPtr mapExp) {

//	check if we got an empty MapExpression
if (mapExp != NULL) {

	// Check if the texture has to be loaded
	std::string identifier = mapExp->getIdentifier();
	TextureMap::iterator i = _textures.find(identifier);
	if (i == _textures.end()) {

		// This may produce a NULL image if a file can't be found, for example.
		Image* img = mapExp->getImage();

		// see if the MapExpression returned a valid image
		if (img != NULL) {

			// Constructor returned a valid image, now create the texture object
			_textures[identifier] = TexturePtr(new Texture(identifier));

			// Bind the texture and get the OpenGL id
			load(_textures[identifier], img);

			// We don't need the image pixel data anymore
			img->release();

			globalOutputStream() << "[shaders] Loaded texture: " << identifier.c_str() << "\n";
		}
		else {
			// Invalid image produced, return shader not found 
			return getShaderNotFound();
		}
	}
	return _textures[identifier]; // this must be outside of the block
}
// We got either an empty MapExpression or the MapExpression returned an empty image, return the "image missing texture"
globalErrorStream() << "[shaders] Unable to load shader texture";
return getShaderNotFound();
}

The routine still terminated with no return value if the image identifier was already found in the _textures map. That's why I moved the return _textures[identifier] outside of the block. Moreover I added a return getShaderNotFound() if the image load failed (i.e. the Image* pointer is NULL). Otherwise the function would exit without return value as well.

 

With the above changes, it works fine, so you were pretty close.

[mohij]

Hm, after seeing the correct way it should be done, it looks quite simple... yeah, next time...

Strange, with those exact changes my radiant still shows white textures instead of Shader not found... could a full recompile solve this?

[greebo]

Possibly not, this must be something else. In fact, I didn't check the "shader not found" texture, I just loaded an existing map (where all the textures were existing), so I haven't watched out for it.

[mohij]

I tested bumpmaps and dynamic lightning and it works.

 

I will try to find the shadernotfound error. And after that I can finally actually start to implement the MapExpressions :-)

 

Completely off topic: should I copy the texture constructor over to ImageExpression::getImage() and remove it?

[greebo]

I'd suggest turning into a (singleton) helper class that can be used by any MapExpression or other classes, like your ImageExpression class does. Definitely keep it in a separate file, as each major class should be kept in its own file pair.

[mohij]

I meant removing the class entirely, as the only place it is used is the ImageExpression::getImage().

But it's also no problem to keep it, I just thought that it might be nicer to remove it.

 

I'm not sure what you mean with turning it into a helper class, it already is a nice small class, isn't it? And how could it even work as singleton, there could very well be more than one of it, or am I mistaken?

[greebo]

Sorry, maybe I misunderstood what you are intending to do. You're referring to the DefaultConstructor class, aren't you?

 

I first thought you wanted to move the code out of the class into the ImageExpression::getImage() method (basically inlining it), which would make it unusable for other classes (MapExpressions).

 

We can of course rename it to make its purpose clearer, but I would keep the task of loading the Image from the VFS separate. So, I'd say we leave it for now.

[mohij]

You understood correct, but anyways, I'll just leave it the way it is. I think this stupid white texture bug is more important atm.

[greebo]

Feel free to upload your current progress to FTP, if you're stuck.

[mohij]

Well, I didn't change anything (except code to find the error). What I found out: getShaderNotFound() is called. I can't check if it is called as often as with old code, since current svn is broken, in case you don't know:

radiant/csg.cpp: In function 'Face* Brush_findIf(const Brush&, const Predicate&)':
radiant/csg.cpp:214: error: operands to ?: have different types

 

Also strange is where such a white texture could come from, as it is a valid image something must have produced it, no idea where it should come from.

 

I'm still looking into it.

[greebo]

SVN is broken? That's strange, I did a full recompile before committing the recent changes. I'll look into that.

 

I don't know exactly but it might be that the white texture is chosen by OpenGL itself?

 

edit: There is a comment regarding gcc 4.1 next to that line, but it seems that this function is unused anyway, so I'll just remove it.

 

edit2: Changes are committed, I'll switch to Linux later on and check that (if you're not faster than me) - I have some work opened here in Windows so I can't (or want to) reboot right now.

[mohij]

Works with the newest changes

[greebo]

Ok, thanks. How's it going with the getShaderNotFound() issue?

[mohij]

Just started again, and I am completely puzzled now. I made getBinding only return getShaderNotFound, so my changes are completely out of the job, and still the blocks are white.

 

edit: Thinking about it a bit more, that obviously means the error must be in loadStandardTexture, and I even found some changed stuff, I think I'll have solved it soon.

[mohij]

Found the mistake. In loadStandardTexture I produced the image via a MapExpression, which uses the DefaultConstructor. The old version uses the FileLoader. It seems the DefaultConstructor can't handle .bmp images.

[greebo]

Ah, yes, the default constructor is designed for the game-supported textures, which are JPG, TGA and DDS, everything else is ignored by the DefaultConstructor.