Understanding the Code

[Springheel 4678]

From my knowledge it's fine to use \n in the XData strings - I don't think there is another way of adding line breaks to xdata values.

 

You can use \n as well as just an empty quote line ("") to leave a line break in my experience.

[STiFU 768]

Hmn, I think I saw a warning somewhere.

 

Ah, I guess I know where my confusion comes from. You're talking about actually putting the String "\n" inside an XData definiton and I was thinking about actual linebreaks, which break the XData definition as far as I know. If we can't find anything in the DefTokeniser that might cause this misbehaviour we can still alter XDataLoader::readLines(). That shouldn't be a problem.

 

The changes you did to the GuiSelector are cool! We should set the ModalProgressDialog to a fixed size though. As soon as a wider gui-name shows up, the window also becomes wider...

[greebo 86]

Hmn, I think I saw a warning somewhere.

A compiler warning?

 

Ah, I guess I know where my confusion comes from. You're talking about actually putting the String "\n" inside an XData definiton and I was thinking about actual linebreaks, which break the XData definition as far as I know. If we can't find anything in the DefTokeniser that might cause this misbehaviour we can still alter XDataLoader::readLines(). That shouldn't be a problem.

I can adjust the DefTokeniser, as this is how the Doom 3 parsers work as well. They can deal with \n, \t, etc. and so should DarkRadiant.

 

We should set the ModalProgressDialog to a fixed size though. As soon as a wider gui-name shows up, the window also becomes wider...

This is not a problem, is it? Anyway, I can strip off the gui path and only display the filename, which should be narrow enough generally.

[STiFU 768]

A compiler warning?

Ah now I remember. The warning was inside Doom I think and it was shown, when there were actual linebreaks instead of "\n" strings in the XData definition. So nevermind...

 

This is not a problem, is it? Anyway, I can strip off the gui path and only display the filename, which should be narrow enough generally.

Of course it's no problem, just a minor cosmetic tweak.

[STiFU 768]

The new solution with the Filevisitor of the GuiManager is simply beautiful. Only parsing the def on demand is much better!! Took me some time to figure out how that thing actually works and why the progress bar of the importing continues where you left of.

[greebo 86]

The new solution with the Filevisitor of the GuiManager is simply beautiful.

Thanks. Though there's always something to improve, I'm sure. Just traversing the filesystem and collecting the names is rather cheap, it was better to collect the filenames during startup and expose an foreach method. The GUI parser itself is not really designed for speed so I delayed this step until it's absolutely necessary.

 

What's still missing is the "reloadGUIs" command, the current setup doesn't support it yet.

[STiFU 768]

Hmn should be easily done by extending the visitor with the GuiInfo parameter. I'd first call findGuis() and then use another visitor class that sets NOT_YET_LOADED on loaded Guis and calls the reload of the gui. I can do that if you want.

 

Aside from that, we just need to add the Readable Editor settings to the preferences menu and tweak the ui and we're all set for the initial release. Nice!

 

Had a very brief look at how to get the preferences done. Ended up with some boost exceptions and went to bed. Does getPage() also create the pages if they are not available yet? And can I just randomly add registry keys or do I have to define them somewhere before?

[STiFU 768]

Ok, I just added the ReadableReloader. It only reloads definitions that were already loaded and also determines their type while doing so. Of course it could be optimized quite a bit, but I didn't want to expose any more methods of the GuiManager to the public Interface. I've got a problem with adding it to the main menu though. I get a Gtk warning "gtk_menu_shell_insert: assertion 'GTK_IS_MENU_ITEM (child)' failed" and the "Reload Models" menu items aren't visible anymore. I've gotta start learning now, so maybe you could look into that?

 

Edit: I did add a reloadGui() method now to the public interface and changed the visitor a little again. With the changes both the Reloader and the Populater have been optimized.

[greebo 86]

The menu item warning was due to some sort of race condition. The reloadModels command was not yet registered in the EventManager, but the MenuManager requested it in response to the insert() call. I've moved the menu item creation to the onRadiantStartup() implementation of RadiantEventListener, this is safer.

 

I also fixed the crash you were getting when trying to construct the preferences. There was an unhandled boost::bad_lexical_cast in SerialisableWidgets. There are some convenience methods in libs/string/string.h, in case you ever need to convert integers to strings and vice versa.

[STiFU 768]

Thanks. I will only have to alter the save method now to use those settings.

 

I've been using boost::lexical_cast all over my code, but I always added a try-block, so that shouldn't cause us any trouble, right?

 

I have had a look at your revisions now, as I always do. (I don't do this to control what you have done there by the way, but rather in hope to learn something new, which I do most of the times) I guess I should get rid of my habit to copy-paste your code, looking at your corrections on the ReadableReloader...

[greebo 86]

Let me know when you're done with the changes, I think we're almost ready to merge the branch back to the trunk and prepare another pre-release build for our mappers.

 

Is the issue with the "source_material/tdm_page" xdata resolved?

[STiFU 768]

Yeah, it's more or less fixed. That definition consists only of a single title and body statement and the importer couldn't cope with that. I fixed that in a fashion that it returns with import failed now. If we want to support such definitions porperly, we'll need to refactor the XData class. Some more support could probably be squeezed out of the current importer, but I don't think it's worth the effort.

 

By the way, you put _guis.clear() in findGuis(). This makes the whole checking for NOT_IMPORTED in the ReadableReloader redundant and nothing will be reloaded, because all guis are NOT_IMPORTED then. I added findGuis() to the reloader class just to make sure also new files are loaded. Didn't want to revert the whole _guis map. This is also the reason why I use insert() instead of operator[] for adding the guiInfo to the map. Already loaded data remains untouched then.

 

I'll add the save-support this evening then. I also just thought of something else regarding the saving process that needs fixing. If you import an XData def, your definition will be stored in the file where the XData definition was found, but this is not generally wanted, so I am thinking if the user imports an XData and changes its name afterwards, the filename should revert to the mapname again.

 

But what about the preview? You didn't comment on my suggestions until now. I don't want to push you anywhere, but I'd really love to see the preview cropped to the dimensions of the background windowdef, as well as a different background color.

[greebo 86]

By the way, you put _guis.clear() in findGuis(). This makes the whole checking for NOT_IMPORTED in the ReadableReloader redundant and nothing will be reloaded, because all guis are NOT_IMPORTED then.

Ah, yes, I'll remove the clear() then.

 

But what about the preview? You didn't comment on my suggestions until now. I don't want to push you anywhere, but I'd really love to see the preview cropped to the dimensions of the background windowdef, as well as a different background color.

What's wrong with black? That's also the colour used by the D3 engine when rendering GUIs, so a D3-compatible GUI renderer should do the same.

 

The cropping is a similar issue - GUIs draw in a 2D space of 640x480, it's just that the backgrounds of our readables are set up in the left upper corner (the in-game script is taking care of the contents positioning each frame). So the preview is correct in itself, I'm a bit hesitant to change it to the size of some hardcoded windowDef.

[STiFU 768]

Well the background color is not really bothering me, we can definitely keep it like that if you want. I agree that the general purpose gui renderer should render the guis just like Doom does, but for the purpose of proper editing and a good visibility of the actual rendered text, I think it would be very neat if the guiView rendered just the readable and not those "black borders". The editor would look much more finished then. Wouldn't it be possible to create an inherited class implementing this, so that we still have a precise gui-renderer available?

 

In the end, the user does not care whether the gui is rendered 100% according to Doom's gui-rendering. He just sees the readable with black borders and thinks that it'd be nicer to have the readable bigger instead of big black borders.

[greebo 86]

Yes, a subclass of GuiView should be able to pull off something like that. Maybe I'll get around to it tomorrow.

[STiFU 768]

Big thanks in advance!! The vertical display area should be adjusted to the windowheight and the width calculated according to the window def of the background I guess.

 

I have just been thinking a little about the release. Since people will need the updated readable def for the editor to work, we could make it a nice double feature release. TDM 1.01 + DR 1.2! Would be a nice surprise for some people!!

[greebo 86]

I have just been thinking a little about the release. Since people will need the updated readable def for the editor to work, we could make it a nice double feature release. TDM 1.01 + DR 1.2! Would be a nice surprise for some people!!

Well, yes, it would be nice. But you realise that we're kind of short of people working on the main release, let alone work on a simultaneous release of DR+TDM. If the past is anything to judge by, the workload will come back down to me, and I'm definitely not going to rip my ass open this time again. Originally I had promised to keep my head out of the TDM 1.01 release, but it's kind of disappointing to see that it is not going very far. I have a girlfriend and a newborn son, a full-time job, plus a couple of other things and hobbies going on. It's just not fun to always be the one who is pulling the cart out of the mud.

[greebo 86]

The ReadableGuiView specialisation is now in SVN. There is only a limited benefit as many background images are larger than the actual image content (the images have transparent borders too).

[STiFU 768]

Ah, what a shame. But at least it works well with one-sided readables. Thanks Greebo. Only thing missing: The widget width is not adjusted. Don't know if that's even possible. Gonna look into that a little.

 

Well, yes, it would be nice. But you realise that we're kind of short of people working on the main release, let alone work on a simultaneous release of DR+TDM. If the past is anything to judge by, the workload will come back down to me, and I'm definitely not going to rip my ass open this time again. Originally I had promised to keep my head out of the TDM 1.01 release, but it's kind of disappointing to see that it is not going very far. I have a girlfriend and a newborn son, a full-time job, plus a couple of other things and hobbies going on. It's just not fun to always be the one who is pulling the cart out of the mud.

I understand of course and I am sorry that I even wormed such a speech out of you. I was just thinking that the new DR release is likely to be done first (unless you want something else in besides the Readable Editor) and that we hold it back until the TDM update is done too. It also wouldn't require any more effort from your side I guess. I'll create new pre-release builds for the beta mappers today and post them, so that it can be tested and if everything works well, we can create the release build and wait until TDM 1.01 is finished.

[greebo 86]

Ah, what a shame. But at least it works well with one-sided readables. Thanks Greebo. Only thing missing: The widget width is not adjusted. Don't know if that's even possible. Gonna look into that a little.

Tried that, but this doesn't work so well. If the widget is taking 640 pixels by default and is later resized to 512 pixels by the code, it will still remain at 640 pixels, the additional space is not re-claimed by the left pane.

[greebo 86]

I'm going to merge the readables branch back into the trunk in the next few hours. Be sure to switch your working copy once this is done.

[STiFU 768]

It works pretty well with a widget_size_request() actually, while only requesting a certain width. Check it out! Had to add searches for other windowDefs as well though, because backgroundImage is not defined in all guis. Also fixed some other issues...

 

I am happy about how everything works now. I am compiling the next pre-releases now then, ok?

 

Edit: Argh, just committed some out-commented code...

[STiFU 768]

Ok, will switch then. Are there any changes on the trunk that aren't present on the readables branch? If so I should rather wait with compiling the new pre-release builds I guess...

[STiFU 768]

There's a problem with the release builds. The XdFileChooser makes DR crash. It does not crash if you browse the guis first. And the x86 version still crashes upon opening the XDataSelector. The weird thing is that this doesn't happen if I launch the release build from within Visual Studio in both cases.

 

I'll look into that tomorrow, after my exam. Can't afford to spend any more time on this right now...

[greebo 86]

Hm, the XDataSelector is still crashing on me when I select the source_material/tdm_page xdata.

 

Couldn't reproduce the other two issues you mentioned in release x64 here.