Itches, Glitches & Anything Else...

[OrbWeaver]

Generally I use savedPos.empty() rather than savedPos == "", but I don't think it should crash. Are you absolutely sure it is that line which is crashing, and not the call to getKeyValue()? Does it actually throw an exception (you should get a console printout with its details if so), or is it just a general crash dialog?

[greebo]

Right, there's no runtime error message, so it's just a plain crash. The getKeyValue() doesn't crash, because I have std::cout statements right before and after this call. First I thought the position list could've been used uninitialised, but that's not the case, I just checked. I'll have to look a bit further, I reckon.

[greebo]

I peppered the routine in question with std::couts:

if (entity != NULL) {
	std::cout << "1";
	const std::string savedPos = entity->getKeyValue(_posKey);
	std::cout << "2";
	if (savedPos != "") {
		std::cout << "3";
		// Construct the vectors out of the std::string
		_position = Vector3(savedPos);
		std::cout << "4";
		_angle = entity->getKeyValue(_angleKey);
		std::cout << "5";
	}
	std::cout << "6"; << ---- This gets executed
}
std::cout << "7"; <<--- this not

That's the output:

Positions: 10
Before load...1234567after load...
Before load...1234567after load...
Before load...1267after load...
Before load...1234567after load...
Before load...1234567after load...
Before load...1234567after load...
Before load...1267after load...
Before load...126

WTF? Is this some kind of stack corruption? It seems it crashes on leaving the scope.

[greebo]

Ok, I replaced the method with some test code like this:

std::cout << "1";
// Sanity check
if (entity != NULL) {
	std::cout << "2";
	entity->getKeyValue(_posKey);
	std::cout << "3";
}
std::cout << "4";
return;

Now it's crashing on the getKeyValue() call, so I'll dig further into this call and see if I can nail it down.

[greebo]

This seemingly cured the problem (see entitylib.h):

 

I changed this

 

return m_eclass->getValueForKey(key).c_str();

to

 

return m_eclass->getValueForKey(key);

 

As the getKeyValue() returns a std::string, and the getValueForKey() does so as well, I ditched to c_str(). This seemed to be the problem, although I don't know exactly, why the double conversion std::string>c_str()>std::string would pose a problem.

[OrbWeaver]

Yes, that would cause a problem, because m_eclass->getValueForKey() returns a string by value, so a temporary will be constructed after that statement, which you are then calling c_str() on. This means that when the temporary is destructed the char pointer is no longer valid.

 

This is why we don't use C strings if possible.

[greebo]

One would think that the c_str() generated from this temporary string would immediately be used to construct the returned std::string of getKeyValue() and this value could be safely returned.

 

Obviously this does not always work, as I could sometimes start DarkRadiant two times in a row without crash. I wonder why this hasn't occurred earlier.

[OrbWeaver]

One would think that the c_str() generated from this temporary string would immediately be used to construct the returned std::string of getKeyValue() and this value could be safely returned.

 

I think the problem is that the c_str() is already invalid, because the returned std::string object is not saved.

 

If you do

 

std::string temp = functionReturningString();
return std::string(temp.c_str());

 

this should be OK, but I suspect that

 

return std::string(functionReturningString().c_str());

 

is undefined, because the return value of functionReturningString is an std::string which is not saved and therefore immediately destructed, meaning that the c_str() pointer is pointing to an invalid location.

 

Obviously this does not always work, as I could sometimes start DarkRadiant two times in a row without crash. I wonder why this hasn't occurred earlier.

 

More to the point, I wonder how I managed to write such a function. Must have been half asleep or something.

[greebo]

More to the point, I wonder how I managed to write such a function. Must have been half asleep or something.

There was probably some legacy stuff like CopeidString involved forcing you to use c_str().

 

Still it's surprising me, that it was functioning more than half of the time, because it's a pretty central entity method and it gets used a lot (my screen was full of std::cout debug outputs during startup). Maybe it's up to the compiler to decide how exactly the return value is constructed (before or after the getKeyForValue() return value is destructed).

 

I'm still believing that using such a construct should not cause a crash, because you should be able to use the result of a nested function return to (again) construct the return value, but assumingly we're walking on the border-line here.

[OrbWeaver]

Maybe it's up to the compiler to decide how exactly the return value is constructed (before or after the getKeyForValue() return value is destructed).

 

I think that's exactly it. As long as the arguments to the return value's constructor are evaluated (which is the const char* in this case), there is no requirement for the compiler to keep alive objects which they depend on if those objects do not have some other reason for remaining constructed. Basically if you use c_str() you have to ensure that the std::string object used to generate it remains alive for as long as you need the char pointer.

[greebo]

It would be interesting to know how this looks like in assembly. The problem will most probably become obvious there, but you stand hardly a chance when looking at the C++ source.

 

A nice and hard lesson learned.

[sparhawk]

All compilers, that I know of can produce assembly language output. In Visual Studio there is an option where you can switch on "Assembly, Source and machine Code". With gcc you can use the /s option. Since the C code is put in as comments, you can easily find out where this code is and take a look at it. In fact I made it a habit to always generate an assembly file when I compile, because this way I can often find problems that are not always obvious from the high level language. I also found several compiler bugs this way. I found a compiler bug a few weeks ago in Visual Net 2003 where this code didn't work:

something = Inventory()->Function();
someother = Inventory()->Fkt(something);

 

and I had to rewrite it to this:

inv = Inventory();
something = inv->Function();
someother = inv->Fkt(something);

 

because the optimizer seemd to screw up the result of the Inventory() call. Maybe you have a similar problem with your embedded call?

[greebo]

Maybe, but somehow I don't feel like going through the hassle of changing the compiler options right now, but still it's nice to know that the assembly option exists, if I'll ever need it.

 

How can you view the assembly code once it's created? Is it produced as cleartext assembly source or do you have to use a disassembler?

[sparhawk]

It's simply a sourcefile.

 

	TITLE	d:\work\CCM\src\CcmAboutDlg.cpp
.386P
include listing.inc
if @Version gt 510
.model FLAT
else
_TEXT	SEGMENT PARA USE32 PUBLIC 'CODE'
_TEXT	ENDS
_DATA	SEGMENT DWORD USE32 PUBLIC 'DATA'
_DATA	ENDS
CONST	SEGMENT DWORD USE32 PUBLIC 'CONST'
CONST	ENDS
_BSS	SEGMENT PARA USE32 PUBLIC 'BSS'
_BSS	ENDS
$$SYMBOLS	SEGMENT BYTE USE32 'DEBSYM'
$$SYMBOLS	ENDS
$$TYPES	SEGMENT BYTE USE32 'DEBTYP'
$$TYPES	ENDS
_TLS	SEGMENT DWORD USE32 PUBLIC 'TLS'
_TLS	ENDS

...

 

; 489  : 			
; 490  : 			// draw the bitmap
; 491  : 			m_dcCredits.BitBlt((m_rectScreen.Width() - bmInfo.bmWidth) / 2, y, bmInfo.bmWidth, bmInfo.bmHeight, &dc, 0, 0, SRCCOPY);

 0149c	68 20 00 cc 00	 push	 13369376; 00cc0020H
 014a1	6a 00		 push	 0
 014a3	6a 00		 push	 0
 014a5	8d 4d a4	 lea	 ecx, DWORD PTR _dc$209032[ebp]
 014a8	51		 push	 ecx
 014a9	8b 55 c0	 mov	 edx, DWORD PTR _bmInfo$209031[ebp+8]
 014ac	52		 push	 edx
 014ad	8b 45 bc	 mov	 eax, DWORD PTR _bmInfo$209031[ebp+4]
 014b0	50		 push	 eax
 014b1	8b 4d e0	 mov	 ecx, DWORD PTR _y$[ebp]
 014b4	51		 push	 ecx
 014b5	8b 8d 30 ff ff
ff		 mov	 ecx, DWORD PTR _this$[ebp]
 014bb	81 c1 98 00 00
00		 add	 ecx, 152; 00000098H
 014c1	e8 00 00 00 00	 call	 ?Width@CRect@@QBEHXZ; CRect::Width
 014c6	2b 45 bc	 sub	 eax, DWORD PTR _bmInfo$209031[ebp+4]
 014c9	99		 cdq
 014ca	2b c2		 sub	 eax, edx
 014cc	d1 f8		 sar	 eax, 1
 014ce	50		 push	 eax
 014cf	8b 8d 30 ff ff
ff		 mov	 ecx, DWORD PTR _this$[ebp]
 014d5	81 c1 d0 00 00
00		 add	 ecx, 208; 000000d0H
 014db	e8 00 00 00 00	 call	 ?BitBlt@CDC@@QAEHHHHHPAV1@HHK@Z; CDC::BitBlt

; 492  : 			
; 493  : 			dc.SelectObject(pOldBmp);

 014e0	8b 55 b4	 mov	 edx, DWORD PTR _pOldBmp$209033[ebp]
 014e3	52		 push	 edx

...

[greebo]

Cool, perhaps this will come in handy someday.

 

Man, the last time I coded in assembly was about 13 years ago!

[sparhawk]

I do it every now and then, mostly as a hobby, because I like assembly. But I'm quite rusty myself as I don't have any real projects to do with it.

[greebo]

Orbweaver, I'd like to have a common TextureSelector object and noticed the current LightTextureSelector class which looks pretty versatile.

 

The only thing that needs to change is the std::vector of allowed prefixes which would be just "textures/" instead of the light-specific prefixes.

 

Can I go ahead and extend this class by a method allowing the client to specify custom prefixes? It could be something like

void addTexturePrefix(const std::string& prefix);
void clearTexturePrefixes();

 

Or do you suggest something different?

 

edit: Ah, I just saw that the infotable is showing light-specific texture info too. Hm. Maybe I'm better off copying the class to TextureSelector and customising it.

[OrbWeaver]

Yes, I had the same idea and then realised that same problem.

 

What you could do, is extend the class to accept both a prefix and callback function/object that is called when a texture is selected. This callback object would be passed the name of the texture and a GtkListStore* that it was required to populate with information related to the texture. This means that the light-related code could pass a callback object that extracted light information, and your code could pass a different callback that would extract generic information.

[greebo]

This is probably the most elegant solution resulting in the least amount of duplicate code. I will take a look at it.

 

I need this for the Surface Inspector as well as for the Find&Replace Texture dialog, where I plan to display an ellipsis button [...] right next to the entry field.

[OrbWeaver]

One thing to watch out for if you haven't already discovered -- the texture selector does not do recursive tree construction, it only has a single layer of textures under each prefix. If you want full recursive evaluation like the Media Browser you will need to modify it to use a gtkutil::VFSTreePopulator to create the tree.

[greebo]

Thanks for the tip, I will watch out for it.

[greebo]

Ok, the ShaderSelector / ShaderChooser is finished. I replaced the LightTextureSelector with the aforementioned. The textures are listed hierarchically and the preview is capable of rendering both editor images and lightfalloffs.

 

The SurfaceInspector is already using the ShaderChooser - the selected shader is displayed in realtime (with the option of hitting "Cancel" to fall back to the previously selected shader).

 

I'm now ready to fix the Texture Find & Replace dialog, which seems to be disfunctional.

[OrbWeaver]

Excellent, code reuse is what we want.

[greebo]

The new Find & Replace dialog is as good as done, I just have to do a full recompile before I commit the changes.

 

[OrbWeaver]

That looks great. The only thing I would change is the bold label -- since this is just a duplicate of the title bar above it, you might as well drop it and simplify the layout slightly.