Jump to content
The Dark Mod Forums
Sign in to follow this  
mohij

MapExpressions

Recommended Posts

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

Share this post


Link to post
Share on other sites

Are the MapExpressions working? Can you see any textures?

 

edit: where exactly does the crash happen?

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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?

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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?

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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?

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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.

Share this post


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.

Sign in to follow this  

×
×
  • Create New...