mohij for Dark Radiant coding

[greebo]

Okay, I already repaired this. OrbWeaver already said what had to be done. I guess that was to easy...

So a bigger task would perhaps be better.

 

ps. What about SVN access vs. sending patches to one of you?

 

How about this: Add "Player Start here" option to Orthocontext Menu?

 

OrbWeaver is the project lead, he'll have to decide about SVN access.

[mohij]

I will give this a go.

 

I would really prefer patches atm, as those are my first programming experiences in the main.

[greebo]

I'm fine with either way. You can either send them to me (greebo@angua.at) or post them here, as it's probably a small fix.

 

I'll assign that task to you then.

[mohij]

I can't upload .patch files. So email then.

 

Can I edit the bugtracker and add myself as "assigned to", or has an admin to do this?

[greebo]

I have setup your user account as "developer", so you can assign tasks to anybody and update them as well. I already assigned that one to you.

[greebo]

Thanks for the patch, I already committed it to SVN.

 

I saw you're using spaces as indentation, which is probably not a problem, I just wanted to point out that I used tabs (that are displayed 4 spaces wide) for all my refactoring.

[mohij]

Are there any policies regarding exact coding style?

like

int func() {

vs

int func()
{

Or indent size? Can I use two spaces as indents?

[OrbWeaver]

Patches is certainly the way to go to start with, yes.

 

Regarding the coding style, there are some guidelines at http://www.thirdfilms.com/darkwiki/index.p...oding_standards. I'm not going to beat anyone up over tabs versus spaces (I'm not even sure which I use myself, it depends how the editor converts things), however please don't use 2 spaces since that is harder to read and inconsistent with the 4 that we are using.

 

Similarly with brace styles, line breaks etc -- there are certain conventions I use but these are not formal rules; the most important thing is to make it look readable.

[greebo]

@mohij: Thanks for the patch, this does work fine. And it was quick.

 

I know that I haven't mentioned this before, but I think it should be expanded to take an existing info_player_start into account. Otherwise the map would end up with several info_player_start entities (but only one would be considered by Doom3).

 

There is a class available named EntityFindByClassNameWalker (in entitylib.h) which can be used to search for a specific entity class. If such an entity is found, it should be moved to the specified location instead of creating a new entity.

 

The ideal solution would be to perform this check before the context menu is actually shown, such that you can rename the Menu item caption to "Move player start here" instead of "Add player start here". This could be more complicated, but I guess you should be challenged a bit.

[mohij]

I finished programming this feature. My problem is that radiant segfaults on any operation on entities. As this also happens also without using my new button, this is probably not related to my changes. So I can't test my button (and remove all those bugs that are surely in there ;-).

 

Sometimes it just segfaults other times it shows:

/plugins/entity/eclassmodel/../modelskinkey.h:114
assertion failure: pointer "instantiable" is null
----------------
----------------
Please report this error to the developers

I guess it is related to greebos recent changes.

I have no idea what I should do...

[OrbWeaver]

Are you developing on Linux? That is very similar to what I am encountering.

[mohij]

Yes, I am on Linux. (Gentoo x86, mostly stable)

[greebo]

I will look into this and hopefully be able to fix it this evening.

 

edit: If you have time, perhaps you could check if this occurs in revision 1760 as well? This was before I started refactoring the typecast system.

[mohij]

I tried version 1760 and it works.

[greebo]

I checked for a missing implementation on the relevant classes as OrbWeaver suggested, but all scene::Nodes are deriving from scene::Instantiable as well (apart from BasicContainer, which I think is not used in this case).

 

I added a NULL pointer guard around the critical part in InstanceSubGraphWalker that emits an error message and some information about that node to std::cout, if the node cast to Instantiable fails.

 

Could you check if the HEAD revision is still crashing or which error message is emitted when creating entities? I will setup a dual-boot Ubuntu today, so maybe I can get to it earlier than you, maybe not.

[mohij]

Error message HEAD shows when creating entities:

libs/scenelib.h:739
assertion failure: findInstance: path not found in scene-graph
...

 

Depending on the entity I try to create, I also get the old error.

[greebo]

Ok, there is something weird going on. I've got my Ubuntu up and running now, so I can finally start hunting that bug.

[greebo]

As you can't see the internal developer forums, I thought I'd drop a note here as well: the problem/crash in Linux is due to the dynamic_cast operator not working when used on classes created in the shared libraries like the entity plugin. The cast returns NULL where it is expected to return something useful. This works in Windows, but not in Linux as it seems.

 

OrbWeaver is currently investigating the linker options to find a way around it - if you're feeling adventurous, feel free to play around with the compiler flags as well.

[mohij]

Will take a look.

 

Just some random thoughts:

dynamic_cast() returns NULL when downcast wasn't successful.

MinGW has gcc 3, Linux generally version 4.

[OrbWeaver]

MinGW has gcc 3, Linux generally version 4.

 

That seems to be the root of the problem -- GCC 4 uses address-based type identification wherease GCC 3.3 (presumably) used name-based. This means that without careful usage of linker options, dynamic_cast and other RTTI operations do not work across different DLLs in the same executable.

 

Fixing the linker options is of course easy, it is the knock-on effects in the form of random crashes during initialisation which need further investigation.

[mohij]

I think it is fixed in latest HEAD. But I'm not sure what exactly fixed it.

[greebo]

It seems to be fixed by the -fPIC switch I had to introduce to make it compile-able on my AMD64 system in Ubuntu. However, it still crashes as soon as I try to view the Media Browser / Shader Selectors and during shutdown. And the splash image is missing somehow, which may even be related to the shutdown crash, but that's a wild guess currently.

[greebo]

mohij, sorry for letting you wait this long, we're still struggling with some issues after the scenegraph refactors (it was a bit of an unfortunate moment, the months before we didn't have such show-stopping issues).

 

I promise that I will look into your contribution in the afternoon, when I can test it using my Windows build environment again.

[greebo]

Ok, I could finally take a look at the contribution, it works fine so far!

 

I found a few things I corrected before committing it to SVN:

 

You have an include there pointing into the plugins folder:

#include "../../../plugins/entity/origin.h" // write_origin()

which is certainly not allowed, because everything under the plugins/ tree should be more or less self-contained and cross-references from radiant/ to plugins/ or vice versa are not allowed. It's of course safe to include something from the libs/ and includes/ stuff from both radiant/ and plugins/.

 

I replaced the write_origin() call with this:

Entity* playerStart = walker.getEntity();

if (playerStart != NULL) {
playerStart->setKeyValue("origin", self->_lastPoint);
}

The setKeyValue() and getKeyValue() methods are the direct and convenient way to alter spawnargs on entities. In case you wonder: if you need to delete a spawnarg, just call it with an empty string setKeyValue("origin", ""), for instance.

 

In callbackMovePlayerStart(): I added a NULL pointer check before using the result of EntityFindByClassnameWalker. It's unlikely that it returns NULL, because you made sure that the according menu item is only sensitive when there is an info_player_start entity existing, but I consider it cleaner this way. You never know how if some routines are used by something else you didn't design it for, so they should be capable of catching such cases.

 

Apart from that it all works fine and the header file is properly commented, which is good. You also made the widget member variables more consistent, which is fine as well.

 

I also noticed that the sensitivity of the "Move Playerstart Here" item is disabled if the info_player_start entity itself is selected, due to this

gtk_widget_set_sensitive(_movePlayerStart, info.entityCount == 0 ? TRUE : FALSE);

That's of course not an issue in itself, it is just possible that mappers find that confusing. We can change that later on, if we, the testers or mappers consider this impractical.

[mohij]

It's great to get such feedback, it really helps understanding things more, thanks.

So, I guess I'm free for another task...