[z64555]

WINDOWS Test Build
Compiled on MSVC 2012 update 4
Debug Console Test Build (based on r10475)

github branch
/fork-z64555/debug_console

Short Tutorial of Use:
1. Run the build.
2. Get to the login screen.
3. Press Shift+Enter to bring up the console.
4. Type in and enter help To list available commands and general usage.

You may use the same key combination (Shift+Enter) almost anywhere in the game to bring up the console, this will currently pause game execution until you leave the console.

CAUTION: Usage of the debug console in multiplayer games may crash to desktop or otherwise cause unexpected behavior. This will eventually^TM change.

It is planned to separate the console to a different thread, so that the game may run in the background. There will be a "pause" command to pause/unpause the game.

(rambling drivel follows)

-=Introduction=-

When I first discovered the FreeSpace Open Debug Console in November of 2013, I found it to be the answer to the problem of needing a way with directly interacting with the FSO engine, especially in terms of debugging new features. The debug console allows developers to check how things are going here and there, all that was needed was run a debug build, get past the opening credits, and hit Shift+Enter almost anywhere in the game to access the console. From here, they could monitor the value of a specific variable and even set them to any arbitrary value.

The F1 help overlay, for instance, has a number of DCFs (debug console functions) to adjust the polylines that draw between the help text and the region of button that they're describing. With a bit of work, something like this could be done for the HUD gauges to aide in their design, in-game. A developer could try nudging the ETS gauge here and there and take it for a test run, and not have to end the game, edit a .tbl, and reboot to see the changes. The debug console also allows testing of otherwise difficult or complex aspects of the codebase. It appears a number of multiplayer-related DCFs were implemented by them to test their lag simulation and how well the engine handled the vigors of networking. Without the debug console, these functions are otherwise inaccessible and useless.

However, by the time I found the console, it was ill maintained and little known. The interface was an abysmal 80x25 letterbox on any resolution of the game, and could only scroll down through a page at a time through the help list. Add to that, the console's command line parser had little to no type safety, making it quite easy to CTD over an otherwise small mistake of entering a float when a DCF needed an integer, or a long when it needed a ubyte. If the console was to live up to its potential, something drastic needed to be done.

Like, a complete rewrite of the parser, and a brand new renderer for the console itself.

-=Overview of Changes=-

The parser now uses a syntax and grammar quite similar to what's found in parselo, and features heavy usage of dc_optional_string's and dc_maybe_stuff's. The latter mentioned are, what I think, novel in grabbing optional arguments from the command line.

The console renderer is still in-game at the moment. A future goal for the console is obviously to have its own window, as with the debug_window that spews the fs2_open.log, but that's outside the scope of this project. (a.k.a. I don't have enough time to fool with it just yet)

The console renderer now features some essentials, like a proper word wrap and the ability for tab alignment (although with FSO's non-monospace fonts, that's pretty much pointless at this time), and the ability to scroll through the output history. Also, the console takes up the entirety of the FSO's screen, and is no longer limited to the 80x25 letterbox.

I tried as much as I could to update the existing DCFs to use the new parsing functions, but there may be bits here and there that I might have missed. I've included quite a few doxygen comments, too. Most should be up to date, but again I might've missed some bits here and there. This will change within the week or so.

-=Ending Note=-

Closing out, I'd like to have as many people looking at this as possible, because I feel that the console is just too good of a tool to _not_ use. Thanks for your interest.

[Goober5000]

Ooh, this sounds encouraging.

[z64555]

I've been having some trouble generating a proper .diff. The situation is a bit weird, as I started working on the debug console while in the antipodes branch. I had to do some finagling of things in order to get the .diff to cooperate with the SVN trunk, but it appears issues are still arising here and there.

Please let me know if any rejected hunks crop up, and I'll do my best to resolve them.

[edit] First revision. Looks like the .diff I originally linked missed a couple of added files. I tried going onto dropbox to update, but for some reason I can't access them at the moment. So, until that gets resolved I'll be using pastebin. Just download the paste as a .diff and delete the /* EOF line, please ignore*/ and the very bottom, but be careful to not delete the EOF line that's right above it.

Link: http://pastebin.com/JahgCFSy

Once dropbox gets back online, I'll also include a test build... at which point this thread should probably be moved into the test build subforum (hint hint) 

[z64555]

Updating OP with a test build.

Would a moderator plz move this to the test builds subforum? pretty plz? 

[ngld]

I've tried to compile this on Linux but the gcc aborts with this error:

Code: [Select]

debugconsole/console.cpp: In function 'void dc_do_command(SCP_string*)':
debugconsole/console.cpp:266:49: error: cannot pass objects of non-trivially-copyable type 'SCP_string {aka class std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >}' through '...'
   dc_printf("Command not found: '%s'\n", command);
                                                 ^
debugconsole/console.cpp:274:70: error: cannot pass objects of non-trivially-copyable type 'SCP_string {aka class std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >}' through '...'
   dc_printf( "Ignoring the unused command line tail '%s'\n", command );

BTW, the link to the GIT diff is wrong... get_flags_wiki ?

[Goober5000]

Nicely spotted, ngld.  This is why we need people to do code reviews.

The printf and sprintf functions (and indeed, any function with varargs) cannot be used with classes, including SCP_string.  The line will need to be changed to pass command.c_str() instead of command.

[ngld]

I've fixed most problems (updated diff) and it compiles by now.
However, once you try to run fs2_open, it instantly crashes. The following error message is what I got from valgrind:

Code: [Select]

==7742== Invalid read of size 8
==7742==    at 0x70ECB3A: ??? (in /usr/lib/libstdc++.so.6.0.19)
==7742==    by 0x4A78FE: std::_Rb_tree_iterator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >::operator--() (stl_tree.h:204)
==7742==    by 0x4A783A: std::_Rb_tree<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >, std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*>, std::_Select1st<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::less<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > >, SCP_vm_allocator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> > >::_M_get_insert_unique_pos(std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const&) (stl_tree.h:1333)
==7742==    by 0x4A7222: std::_Rb_tree<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >, std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*>, std::_Select1st<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::less<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > >, SCP_vm_allocator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> > >::_M_get_insert_hint_unique_pos(std::_Rb_tree_const_iterator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const&) (stl_tree.h:1425)
==7742==    by 0x4A6F9E: std::_Rb_tree<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >, std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*>, std::_Select1st<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::less<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > >, SCP_vm_allocator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> > >::_M_insert_unique_(std::_Rb_tree_const_iterator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> const&) (stl_tree.h:1478)
==7742==    by 0x4A6C3D: std::map<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >, debug_command*, std::less<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > >, SCP_vm_allocator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> > >::insert(std::_Rb_tree_iterator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> const&) (stl_map.h:648)
==7742==    by 0x4A68BA: std::map<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >, debug_command*, std::less<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > >, SCP_vm_allocator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> > >::operator[](std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const&) (stl_map.h:469)
==7742==    by 0x4A6102: debug_command::debug_command(char*, char*, void (*)()) (consolecmds.cpp:37)
==7742==    by 0x41B10D: __static_initialization_and_destruction_0(int, int) (freespace.cpp:669)
==7742==    by 0x41B5EB: _GLOBAL__sub_I_Env_cubemap_drawn (freespace.cpp:8942)
==7742==    by 0x916FEC: __libc_csu_init (in /home/tobias/games/fs2/fs2_open_3.7.1_DEBUG)
==7742==    by 0x75B3A94: (below main) (in /usr/lib/libc-2.19.so)
==7742==  Address 0x8 is not stack'd, malloc'd or (recently) free'd

It seems this is caused by "dc_commands[_name] = this;" on line 37 of consolecmds.cpp.
I have no idea what exactly is going wrong here...

[m!m]

@ngld: Your patch does not contain any changes which are not in the code folder, can you recreate the patch so it includes the changes to the Visual Studio project files?

@z64555: I looked at the patch and I noticed some things that you might want to fix:

• You are using plain char* pointers in a loot of places where you should use const char*, always use const char* if you don't actually need to change the string as using char* pointers with string literals and then changing them causes undefined behavior.
• You have a lot of duplicated code in consoleparse.cpp which only differs in that one uses a c-style string and one uses a SCP_string, removing the c-style version and patching all usages of it will make it easier to add internationalization later.
• This is more a personal opinion and nothing set in stone but using a pointer to a variable to pass return values and then not actually checking for a NULL-pointer is dangerous and should in most cases be replaced by a reference as that will also catch many other cases of wrong usage.
• Is there a reason why you are using 8 spaces to indent? I had the impression that 4 spaces or a tab were the "default" style in the FSO code although we don't actually have an official coding style

[z64555]

@ngld: I updated the GIT patch not too long ago, that might've been what confused Pastebin and sent you to a different paste (that's actually a rough draft for a fso cmdline option).

The link should now point to the intended patch.

@m|m:

• Good catch on the char * vs. const char *. I'll see if I get that changed today.
• I was going by the example put forth by parselo.cpp, I wasn't sure what was the prefered route so I went with both until that was settled.
• Hmm, was pretty sure I got all of the nullptr checks in place, but it looks like a few functions slipped through without getting them.
• Uh, where is it 8 space indented?

[m!m]

In my patched checkout consoleparse.cpp is indented with 8 spaces but it might be an issue with the patch files.

Also I just noticed you are using a class called errParse to report parsing errors but you are not deriving it from std::exception which should be done in most cases.

[Goober5000]

The example in parselo.cpp, where there are both SCP_string and char* versions of string processing functions, is because we want to be able to use the functions on both sources.  I'm currently in the middle of a project to convert the mission loading code to use SCP_string, and I use both versions of the parselo functions in various places.

Whether to maintain parallel versions of the functions in this patch depends on the intended use.  Based on m!m's comment "...removing all uses of [the c-style version] and patching all usages of it", it sounds like there are quite a lot of references to both throughout the code, so you should leave both versions in for now.  Focus on getting the functionality working first, and then at some future date, you can refactor the invocations if you need to.

Regarding "using a pointer to a variable to pass return values ", I haven't checked the diff, but is this used in the function parameter list or in the return statement?  If it's in the latter, you would be returning a pointer to a variable on the stack and that is a big no-no.

[m!m]

[...]  Based on m!m's comment "...removing all uses of [the c-style version] and patching all usages of it", it sounds like there are quite a lot of references to both throughout the code, so you should leave both versions in for now.  [...]

I haven't actually checked which version is used, I only noticed that there was quite a lot of duplicated code because of this and I though now would be a better time to unify it to use the SCP_string version instead of waiting and then never doing it.

Regarding "using a pointer to a variable to pass return values ", I haven't checked the diff, but is this used in the function parameter list or in the return statement?  If it's in the latter, you would be returning a pointer to a variable on the stack and that is a big no-no.

I was talking about pointers in the argument list.

[Goober5000]

I haven't actually checked which version is used, I only noticed that there was quite a lot of duplicated code because of this and I though now would be a better time to unify it to use the SCP_string version instead of waiting and then never doing it.

There's no harm in leaving it alone for now.  In contrast, postponing it has the advantage of separating conceptual blocks of work into different patches, and doing it now has the disadvantage of making the diff more complex and risking the introduction of bugs.

[ngld]

I've updated the GIT patch.

• Added c_str() every time a SCP_string is passed to dc_printf().
• Added #include <limits.h> to consoleparse.cpp for INT_MAX.
• Added #include "debugconsole/console.h" to windows_stubs/stubs.cpp because it uses DCF_BOOL.
• Changed dc_command_buf.pop_back() to dc_command_buf.erase(dc_command_buf.end() - 1); since pop_back() is C++11
• Added all new files in code/debugconsole to code/Makefile.am

FSO now compiles and works correctly but the console has strange issues when printing text...

The prompt always displays correctly but once I press enter the last output line (which wasn't visible before) appears on the same line as the prompt...

[z64555]

@ngld: It appears that some functions that use dc_printf don't have the needed '\n' at the end, leaving the printf cursor at the end of the last line printed instead of at a new line. When a new command is entered from the command prompt, the string is pushed onto the output buffer, and thus, collides with the last line.

Hmm, see_all is likely a DCF_BOOL function, so that may be one of my mistakes there. 

[amend1] I'll see if I can get a github repo set up. This exchange of .patch's is just too cumbersome.

[z64555]

Alright, trying to pass along a Git .diff and an SVN .diff proved to be too much of a hassle. I've finally got my git-svn local repo set up so that it can push to either the SVN, the github remote, or my personal fork. Once code review completes I'll merge the debug_console branch into the master branch, and then either Dcommit it into the SVN or pull it into the GIT origin, depending on which remote will be considered the official at the time.

Here's my fork: fork-z64555/debug_console

I just finished doing the merge from the antipodes branch where I originally started work on the debug console. Let it be known that I am never goign to try making a .diff by hand ever again, because it's just too big a pain in the ass to do, and a bigger pain in the ass to try to correct.

Notable changes:

• String parsing functions that stuffed to a char[] have been dropped in favor of the duplicate functions that use SCP_string instead.
• Stuffing functions have been largely changed from accepting a * to a & reference.
• SCP_strings that are used in dc_printf's have been postscripted by the proper .c_str()

[amend] I updated the OP to reflect the switch to the github branch. I'll see if I can upload a debug build of the the newest commit soon.

[amend_1]OP updated again, this time I updated the link to point to the newest build.

[Echelon9]

Hi - Are you able to rebase this against the latest scp git branch? Reading through the comments this is as against the antipodes branch am I correct in thinking?

[z64555]

It was originally based against antipodes, but after some learning experiences I managed to get it based on remotes/origin. That's what's on the fork right now.

[m!m]

The problem is that your debug console branch is not based on any branch from the main repository. The easiest way to fix this is probably to just create a new based in the antipodes branch from the main repo and apply your patch to that branch.

[z64555]

Alright. Don't try to pull anything from my fork. I'm in the middle of some management.

Sanitized the git repo, and set everything up as it should. I've seperated the svn and git repos, so this shouldn't be a problem anymore.