Itches, Glitches & Anything Else...

[greebo]

I get a file conflict during SVN checkout that's probably windows-specific: SVN wants to create a file named libs/gtkutil/GLWidget.cpp, but there is also a file named libs/gtkutil/glwidget.cpp existant in my according folder.

 

Deleting the lowercase file (I suppose that's the old one) doesn't do the trick, because it's restored by SVN update, I guess both files are still in the repository.

[OrbWeaver]

I didn't think of Windows and its case-insensitive filenames when adding that. In fact I might remove that file altogether -- I thought that there might have been something screwy going on with the GLExt widgets which was causing angua's error which would require a new GLWidget class, but since that is not the cause of the error the rewrite is probably unnecessary.

[greebo]

I'm just writing the Texture TokenizerFunc and while looking through DefTokeniser.h I discovered this. I'm not sure if this is a bug or not, but I better post this here:

 

case STAR:

				// The star may indicate the end of a delimited comment. 
									// This state will
				// only be entered if we are inside a delimited comment.

				if (*next == '/') {
					_state = SEARCHING;
					continue;
				}
				else {
					continue; // do SEARCHING
				}

 

In my opinion, this should look like this:

case STAR:

				// The star may indicate the end of a delimited comment. 
									// This state will
				// only be entered if we are inside a delimited comment.

				if (*next == '/') {
					_state = SEARCHING;
					continue;
				}
				else {
					_state = COMMENT_DELIM;	 // ADDED
					continue; // do SEARCHING
				}

Because otherwise, any star that is found within a delimited comment would cause the loop to switch into SEARCHING mode instead of carrying on with COMMENT_DELIM.

[OrbWeaver]

You are right, the state should be reverted, otherwise the string "* blah blah /" would terminate the comment. In SVN.

 

You might be able to use DefTokeniser as-is to parse texture defs, I think the tokenising is the same in both cases (basically C/C++ style tokenisation with comments and quote-protected tokens).

[greebo]

I had this in mind, but looking at the material files I decided to write a separate "MaterialTokeniser.h", as the definitions are a bit differently organised (with many curly braces all through the file). As far as I can tell, a general texture definition (or table) looks like this:

 

<name> <definition block surrounded by curly braces>

 

where the definition block may consist of several stages, that (according to my plan) are to be parsed by a seperate tokeniser (a "ShaderTokeniser").

 

So at the moment I have a working "MaterialTokeniser" that can split a given material file into tokens like in the code above.

 

These tokens (i.e. either a name or a definition block) are sent to another parser, that analyses the actual stages and calls the existing radiant functions to add them to the shader memory. I'm currently working on this ShaderTokeniser.

 

Any suggestions? Do you consider this as too cumbersome?

[OrbWeaver]

Bear in mind that a Tokeniser is only supposed to split the input file up into "tokens", that is, atomic elements of the source code. This includes braces, variables, string or numeric literals, keywords, arithmetic operators and so on. The tokeniser is also responsible for stripping out comments so the actual parser doesn't have to deal with them.

 

From your description it looks like you are confusing "tokeniser" with "parser". You would not use a separate tokeniser for stages or subexpressions in a given piece of source code -- this would be the job of the Parser (which takes the atomic tokens from the tokeniser and uses them to construct whatever data structures are required by the application).

 

I hope this is clear. I might have given the wrong impression by naming the class DefTokeniser, since this suggests that the class is responsible for parsing DEFs (in fact the parsing happens in eclass_doom3.cpp). Perhaps I should have called it Doom3DeclTokeniser or something, to make it clear that it was only responsible for extracting atomic tokens from the source, not for parsing them into meaningful structures.

[greebo]

Hm, ok, I guess I mixed this up. Right now the tokeniser is delivering parts like this (comments are all stripped out, and the strings are trimmed):

 

dtable1

{
wood

diffusemap	models/darkmod/props/textures/dtable1_d.tga
bumpmap		models/darkmod/props/textures/dtable1_local.tga
specularmap	models/darkmod/props/textures/dtable1_s.tga
{
	if ( parm11 > 0 )
	blend	   gl_dst_color, gl_one
	map		 _white.tga
	rgb		 0.40 * parm11
}
{
	if ( parm11 > 0 )
	blend	   add
	map		 models/darkmod/props/textures/dtable1_d.tga
	rgb		 0.15 * parm11
}
}

I understand that a whole block can hardly be understood as an atomic part of the material file, but I have to admit that I find it convenient to feed my parser with parts like this. Is this considered bad programming style?

[OrbWeaver]

Yes, that is the job of the parser, not the tokeniser.

 

The tokeniser should deliver parts like:

"models/mymodel/mytexturename"
"{"
"wood"
"diffusemap"
"models/darkmod/props/textures/dtable1_d.tga"
"bumpmap"
...

 

The parser would then operate on these tokens, generally in a recursive style, e.g.

 

parseMaterialDef();

 

which becomes:

 

name = parseMaterialName();
assertToken("{");
while (nextToken != "}") {
 if (nextToken == "{") {
// recursively parse the sub-stage
 }
 else { 
// do something based on the recognised keyword, otherwise throw exception
 }
}

 

Obviously this is very quick psuedocode and the actual implementation would need to be more complex, but the general idea is to parse by recursively calling functions which assemble tokens into a particular part of the data structure, with all of the parts eventually being assembled into the whole structure.

 

The problem with mixing the tokeniser and the parser is that you get duplication of effort, for instance, each "subtokeniser" will have to handle comments itself, rather than the parser relying on being presented with a comment-less token stream from a single tokeniser.

[greebo]

I see. Thanks for explaining!

 

BIG EDIT: Forget what I posted here before, if you read it. I can see the advantage now - whitespace is the name of the evil...

[greebo]

There is a problem with compiling the current SVN version on Windows. The include "GL/glew.h" is not found by aabb.cpp (I use the latest Sconscript from SVN).

 

I looked into Sconstruct and the paths under useOpenGL() seem to be correct for me. Is there something wrong with the order the environments are defined in?

 

scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
g -pipe -W -Wall -Wcast-align -Wcast-qual -Wno-unused-parameter -Wno-non-virtual-dtor -Wreorder -g -D_DEBUG -Ibuild\de
bug\radiant -Iradiant -Ibuild\debug\include -Iinclude -Ibuild\debug\libs -Ilibs -Iboost.w32\include -c -o build\debug\libs\math\aabb.o libs\math\aabb.cpp
libs\math\aabb.cpp:24:21: GL/glew.h: No such file or directory
libs\math\aabb.cpp: In member function `virtual void AABB::render(RenderStateFlags) const':
libs\math\aabb.cpp:48: error: `GL_LINES' undeclared (first use this function)
libs\math\aabb.cpp:48: error: (Each undeclared identifier is reported only once for each function it appears in.)
libs\math\aabb.cpp:48: error: `glBegin' undeclared (first use this function)
libs\math\aabb.cpp:49: error: `glVertex3f' undeclared (first use this function)
libs\math\aabb.cpp:84: error: `glEnd' undeclared (first use this function)
scons: *** [build\debug\libs\math\aabb.o] Error 1
scons: building terminated because of errors.

[OrbWeaver]

Fixed.

 

I had forgotten to add useOpenGL() to the math libary build options when making the AABB renderable. Unfortunately I tend not to see these errors on Linux because everything is located under /usr/include, so include paths don't need to be specified, but obviously on Windows this doesn't work.

[greebo]

Ok, thanks for that!

 

This week will be quite busy for me, but I hope that I can get some things to work in the evening hours.

[OrbWeaver]

Cool. I guess I'd better write up some coding standards for DarkRadiant now we have a couple of people working on it.

[greebo]

Hm, I still get a linker error. Is there some LINKFLAG missing? The md3model.env is already using the useOpenGL() command.

 

gplusplus -Wl,--kill-at -shared -o build\debug\md3model.dll build\debug\plugins\md3model\plugin.os build\debug\plugins\md3model\md5.os -Lboost.w32\lib -Lbuild\debug\libs -Llibs -Llibxml2.w32\lib -Lzlib.w32\lib -Llibiconv.w32\lib -Lw32\glew\lib -llibxml2 -lz -liconv -lglew32 -lglu32 -lopengl32 -lgdi32 -lmath -Wl,--output-def,build\debug\md3model.def
build\debug\libs/libmath.a(aabb.o)(.text+0x263): In function `ZNK4AABB6renderEj':
G:/Dark Mod/darkradiant/libs/math/aabb.cpp:49: undefined reference to `glVertex3f@12'
build\debug\libs/libmath.a(aabb.o)(.text+0x2b2):G:/Dark Mod/darkradiant/libs/math/aabb.cpp:50: undefined reference to `glVertex3f@12'
...
build\debug\libs/libmath.a(aabb.o)(.text+0x3ee):G:/Dark Mod/darkradiant/libs/math/aabb.cpp:56: more undefined references to `glVertex3f@12' follow
collect2: ld returned 1 exit status
scons: *** [build\debug\md3model.dll] Error 1
scons: building terminated because of errors.

 

Regarding coding guidelines: This is a good idea, I will put them on the Wiki, as soon as you have them ready. At the moment I'm imitating whatever coding style I find in the existing source. Most important to know for me would be some naming conventions, upper and lower case and opening/closing brackets indentation guidelines (after if, else, while, void blabla() statements).

[OrbWeaver]

I don't understand that linker error, all of the relevant GL libraries appear in the link commandline. I will have to try building on Windows and see if I can replicate the error.

 

Emulating the existing coding style is good idea, but make sure you emulate the style in my additions (anything under radiant/ui) rather than the existing GTKRadiant codebase. I want to move away from some of the ugliness in the legacy code, like using C-style functions and char* arrays rather than C++ objects and strings.

 

The naming conventions are basically Java-style - capitalised class names, variables and methods start with lowercase, one class per file pair (e.g. MyClass.h / MyClass.cpp) and class methods rather than C-style functions (Vector3::scaleBy(float factor) not Vector3_scale(const Vector3& self, float factor)). I'm not too bothered about indentation and spacing as long as it is readable and consistent, although I would prefer 4 spaces rather than 2.

[greebo]

...one class per file pair (e.g. MyClass.h / MyClass.cpp) and class methods rather than C-style functions (Vector3::scaleBy(float factor) not Vector3_scale(const Vector3& self, float factor)). ...

So the thing to avoid is multiple classes in one file, that sounds reasonable.

 

Currently I'm working on shaders.cpp, where multiple classes and non-class functions are defined. Should I extract the texture parser from this file and pack it into a separate file and class?

[OrbWeaver]

Currently I'm working on shaders.cpp, where multiple classes and non-class functions are defined. Should I extract the texture parser from this file and pack it into a separate file and class?

 

If it's a new class, yes. If you are editing an existing class, don't worry about extracting it (although you are welcome to if you wish).

[greebo]

Could you reproduce the linker error? I'm a little stuck a the moment, I tried to google about this problem, but could not find anything. Are you using MinGW or cygwin?

[OrbWeaver]

I'm using MinGW. I can recreate the error, but am at a complete loss as to what is causing it. Running nm on the MinGW version of libopengl32.a reveals that all of those symbols do exist, but they have leading underscores (_glBegin@4 etc.). At first I thought that this problem was caused by having the -lmath option after the OpenGL linker options, but changing this doesn't solve anything.

[sparhawk]

Having a _ is normal, as in C all functions are prefixed with an underscore. It's C standard.

[OrbWeaver]

Having a _ is normal, as in C all functions are prefixed with an underscore. It's C standard.

 

That's what I thought. Which raises the question, why in this case has GCC decided to look for symbols WITHOUT a leading underscore? The only thing unusual about this case is that it is the first time I have made OpenGL calls from within a static library. Maybe there is some special treatment that is given to libraries that causes the symbol naming convention to be changed.

[OrbWeaver]

I give up. This makes no sense whatsoever. The md3model and entity build environments are IDENTICAL apart from the source filenames, but one will link these symbols and the other won't.

 

Just blank out the body of AABB::render() in libs/math/aabb.cpp for the time being, and I will find some other way of making the AABB renderable, perhaps using a separate adaptor class in the ModelSelector itself (which may be a better idea anyway - rendering functions are not a natural part of a math library).

[greebo]

Maybe this is some bug in MinGW with this special scenario? We could try to compile it with a different version or cygwin?

[OrbWeaver]

Have a go if you like; however I don't think it is something worth spending too much time on (also I have a feeling the Cygwin + GTK = nastiness). I was tempted to use an adaptor class to start with anyway - OpenGL rendering is not an integral part of the representation of a bounding box and therefore there is a good argument for not having the math library depend on this.

 

What I will probably do is add a class RenderableAABB in radiant/ui/common, which will implement the OpenGLRenderable interface (irender.h) to draw the bounding box (and if that doesn't link, I'll give up programming altogether and become a pastry chef).

[Crispy]

Sounds like a plan, Stan.

 

(Mmmn. Pastry. )