Code formatting - Githubissues

Anthony-Gaudino commented 8 years ago

Hi,

I have been following RBDoom 3 BFG and Open Tech Engine BFG for a long time and now I have time to contribute to this project.

I never worked in a codebase this big and I think it would be better to start doing something simple and easy, so I though I could format the code. I see that OTE is using Artistic Style (AStyle) with parameters on astyle-options.ini but, while reading the code I noticed that some things are not well aligned, especially declarations of variables, comments and macros.

Just let me know how you want the code to be formatted and I will do it. I will also review the astyle-options.ini, I think it can be improved.

ghost commented 8 years ago

Hi Anthony, thank you for showing interest in the project. You might only get an answer from the others by tomorrow as they may be at work or sleeping and handle all the coding of the project.

DanielGibson commented 8 years ago

Reformatting has two major disadvantages:

it makes git blame less useful
it would make merging changes from RBDOOM3-BFG harder - we use the same astyle settings as he does to make sharing code easy.

So you'd have to also convince him of doing your changes (and that still leaves the first problem unsolved).

Anthony-Gaudino commented 8 years ago

@DanielGibson, I'm not proposing a massive change of code formatting, the code would still have the same style set on astyle-options.ini. I would format the code manually where AStyle was not able to do it. I would improve astyle-options.ini (translate to English and make it more readable) and propose that other formatting could be enforced by it.

I can format both RBDoom 3 BFG and Open Tech Engine BFG together so they both will have the same formatting.

I will wait for @kortemik, @RobertBeckebans and @BielBdeLuna to say what they think.

RobertBeckebans commented 8 years ago

Don't waste your time on formatting. Write features :)

Anthony-Gaudino commented 8 years ago

@RobertBeckebans, I know :D, but as I said I never worked with a codebase so big, and want to start slowly, I have some features in mind I would like to see in the engine, but with time I may implement them.

Also, you must know that I'm not the most skilled C/C++ developer around and never worked with a game engine before (I did a C ASCII game on college, tough), so there's a lot of stuff I still have to learn.

ghost commented 8 years ago

@Anthony-Gaudino I'm working on art assets for OTE, but learning C/C++, SDL2, Bullet physics and OpenGL on the side. I want to create a simplistic modern level editor for OTE that creates some level assets in the background on startup to speed up the mapping process. The radiant editors frustrates me after working with UE4.5 and Unity.

Maybe we can work on it together on a seperate branch as a learning project. SDL2, freeglut and glew is quite easy to set up and somewhat easy to work with, the only tricky thing I can think of is how the BSP data and dmapping is handled. So let me know if you would like to partner up on that.

Anthony-Gaudino commented 8 years ago

@Yetta1 I like the idea, but I still think it's too early for me to start implementing new features.

In my mind I have this:

Format code to make it the most legible possible;
Document code (so I and others can understand it better);
Fix issues/improve code;
Add new features.

Have you ever played Cube 2 or Tesseract? Those engines have an interesting in game level editor. If I remember correctly, ID Tech 4 also allows you to enter an editing mode during gameplay.

I know how to use GIMP, Inkscape, Adobe Illustrator and some other image editors, I have also have experience with some 3D software, so I may help you with something related to the assets, but I don't consider myself an artist.

DanielGibson commented 8 years ago

I think formatting the code doesn't help much, the current format is already pretty readable.

Documenting the code.. I discussed that a bit with kordex some days ago and we agree that adding doxygen-like documentation in the code doesn't have much value, but makes merging code from other branches (RBDOOM3-BFG and whoever else develops with the d3bfg engine) a lot harder. However, we think that documenting the engine in general (what is done in which part of the code? how do the subsystems interact with each other?) would probably be helpful for people not familiar with the code base who want to add features (this potentially includes ourselves, there are large parts of the code I've never really looked at). http://fabiensanglard.net/doom3_bfg/index.php is a starting point, but one could go a bit more into details than that. So if you wanna learn about the code maybe writing documentation in a wiki would be a good start?

Those Cube/Sauerbraten based engines are interesting, but fundamentally different to Doom3 (BFG), because they use octrees as the fundamental datastructure, which allows the easy realtime editing as you experienced it, but is also kinda restricting to the kind of geometry you can use in the levels (everything not aligned to the grid is hard).

Old Doom3 had ingame editors that allowed to modify lights, edit sounds and particles and things like that. I'm currently (whenever I have time, on weekends etc) porting/recreating them to OpenTech, see http://imgur.com/a/Z6OsW for early screenshots. I'm not sure yet which editors I will port; definitely not the radiant, it's too complex, but most probably at least light, particles and AF.

ghost commented 8 years ago

@Anthony-Gaudino Yes I've already mentioned Tesseract to the other guys when we spoke about having a realtime level editor with network support. Id Tech 4 relies on mapping to be done in radiant, editing within the game is very limited as the level requires the map to be built otherwise it creates leaks.

The main idea behind the editor I want to create is to have a layer system, similar to what DarkRadiant has, however some layers will have precreated assets such as a skybow with day/night cycle and lighting to save some time on building and get to the mapping. Another thing which makes makes mapping frustrating in Radiant is have to create scripts for all the material shaders, models etc, instead i'd like a drag and drop approach in the editor which generates the code and saves it out to the correct directories, with an inclusion of option flags to control the properties for the models, e.g. translucent/metalic/liquid/etc shader, static/rigid/dynamic/actor for models and so on. It will save a lot of time in the development pipeline for game developers.

As for documentation, OTE already has documentation page https://modwiki.xnet.fi/, I can not even express how grateful I am for it, a lot of effort has gone into it.

I'm using Blender, GIMP, Paint.Net, Inkscape and FL Studio for the art assets, most of the 3D models are done, I still need to finish UV unwrapping, rigging, animating and scripting.

I'm learning from these tutorials on how to use SDL and OpenGL. http://lazyfoo.net/SDL_tutorials/

DanielGibson commented 8 years ago

Regarding learning C++: Learn C first if you ever want to understand what's actually happening in C++.

ghost commented 8 years ago

@DanielGibson D: Y U NO TELL about the light editor? giefs cvar prease.

DanielGibson commented 8 years ago

because it's not done yet. it's not in master and even the status I currently have can't save map files yet and is generally buggy.

ghost commented 8 years ago

Good job :+1:, i really like it, It will be useful. Since I can't use a fleshlight anymore in unlit areas of the map. Building a low latency linux kernel takes soooo long.

kortemik commented 8 years ago

Hello and welcome @Anthony-Gaudino . It's always delightful to find new minds interested in this project family.

Let me be bit strict and brief about the proposal you had about the code base:

Format code to make it the most legible possible; The Astyle at the moment is ok enough and migrating has drawbacks as already stated. Not that I'd really like to see style stated in this book with all the initialization lists and et cetera. But that's a day dream not something that is worth putting hours in at the moment.
Document code (so I and others can understand it better); As @DanielGibson stated high level documentation would be nice. In case you want to go with Doxygen it needs to have minimal impact on the actual code. So if you want to really use it go for it but please keep the changes at minimum, it's rather better have just doxygen generate the code references and after that you can reference them in the high level documentation via links or so on. This would be my design.
Fix issues/improve code; Please do, this is where I would start getting familiar with the code. We do have a Coverity account which lists out the simple problems with the code. Really handy and a soft landing to fixing issues. And to make it even more into a sweet candy, in case you get fixing those, we are delighted to review the fixes and suggest improvements on them. The page is here https://scan.coverity.com/projects/4341?tab=overview please create an account and request access.
Add new features. I'd go with the above tasks first, adding new features is quite of a task to start with and will get your motivation down. Even @DanielGibson is avoiding our new gui system because docs are scarce and the same applies to everything else in general. @RobertBeckebans has the most knowledge about the engine but he is very tied up with other things and DG and me do this as a hobby on late hours (like now 1:30AM) so guiding you on both high and low level design on the code would be left with too less focus so I don't really recommend starting here if C++ is not yet too familiar.

I really recommend the book which of example I did recommend previously. In case you choose to go with the Fix issues/improve code path, it's a neat reference when it comes to fixing the issues and get's you familiar what happens and why the static analyzis said it's wrong.

I hope we didn't scare you off with all this rambling. -kordex

kortemik commented 8 years ago

I saw already that you had request on coverity, it's now allowed.

DanielGibson commented 8 years ago

But note that coverity might have a lot false positives, so before "fixing" the "problems" there, try to find out if they're really a problem (ideally try to reproduce a crash or whatever from the problem). And make sure to do one commit per fix, so it's easier to remove fixes that don't seem good (or do git bisect on them later if we assume some fix caused a bug).

Regarding doxygen, if you wanna use it, whatever (but I'm not sure we'd merge the changed code with those annotations to master), but IMHO writing directly to a wiki (possibly linking code from the repo there - make sure the links are from a specific revision, not a branch like master, because in branches the line numbers may change) could work as well.

BielBdeLuna commented 8 years ago

it's great to have new people come aboard

I agree on the formatting of the code, doing it might make even more difficult to merge new code. and maybe even adding doxygen might prove an issue when merging code.

on the documenting of the engine, this should be a perfect project, and it could help you understanding the code, there is already the modWiki page that explains how to mod with the engine, it's not the internals but it might prove a valuable resource: modwiki.xnet.fi

on new content for the engine, maybe the easiest at the moment, and also being a project in itself, is adding ogg to the engine, as there is the dhewm3 engine that already has it, albeit the code changed between idTech 4 to idTechX, because idTechX binarizes content, Ogg is necessary for music, or long sounds like room tone sounds and currently idTechX lacks support for it for unknown reasons.

@Anthony-Gaudino it's great to have new people, welcome!

@DanielGibson nice light editor! maybe I would make a suggestion, it might prove useful to also see the boundaries of the light so we could use to minimize many lights touching.

kortemik commented 8 years ago

@DanielGibson I rather meant using Doxygen without any repo commits, just to index the code so that links would work now, and in future as well.

Anthony-Gaudino commented 8 years ago

I'm still finishing setting up my system.

I installed Debian 8 64bit and just finished building OTE as a test. Updating to Debian 8 created an issue for me because I need AMD FGLRX for my graphics driver and Gnome 3 is not supported on it (https://www.debian.org/releases/jessie/amd64/release-notes/ch-information#gnome-fglrx). So I may need to install another desktop environment to run DOOM 3 BFG.

On my OTE fork, I will create a branch where I will put a copy of RBDoom3 Master and another branch to put OTE Master, so I can keep them updated in relation to both projects. Then I will create two more branches from those copies where I will work on code formating, this way both projects will be code formated individually and if the project owner wants, he can merge the changes. I believe that modifying both projects at once will make it easier to merge changes between them.

Will 80 characters be the limit width of a line of code?

I have a question for users of Eclipse CDT. When I run cmake-eclipse-linux-profile.sh the generated Eclipse project is a bit messy, I created an Eclipse project by hand following the instructions in this page http://www.cmake.org/Wiki/CMake:Eclipse_UNIX_Tutorial#Option_2:_UNIX_Makefile_Generator, I was able to build and debug OTE, but there's more to setup to do on it. Do you think that creating an Eclipse project this way is a good idea? One of the advantages is that I can configure the project and share it, so settings like code formatting will already be configured for the project.

ghost commented 8 years ago

@Anthony-Gaudino I'm also having issues with AMD FGLRX on linuxMint, the 3D acceleration is enabled in xorg.conf and glxgears runs at 60fps in sync with the screen refresh rate. I removed FGLRX and installed the open source AMD drivers, however Doom3 and OTE both can't seem to see that 3D acceleration is enabled. There seems to be a lot of issues with AMD cards on linux, i've never had issues with nVidia drivers. Radiant and Blender runs fine however. You may have to enable the features manually in the xorg.conf file, there is also some other projects which help for acceleration like Gallium nine and DRI Prime.

https://xpressrazor.wordpress.com/2013/10/08/enable-and-use-open-source-radeon-drivers-in-a-muxless-hybrid-graphics-intelamd-setup/

Just be careful as it can break xorg if setup incorrectly.

You'll have to talk to Daniel about Eclipse, as it's the IDE he is using, i've tried setting up the project on Eclipse but have had no luck so far.

kortemik commented 8 years ago

@Anthony-Gaudino scripts/astyle.sh will take care of the formatting in means of line width and the rest, don't worry about it.

About commiting to both branches, if you create a branch on OTE, it's easy to backport the changes for RBDoom3 so no need to commit on both as long as you keep the fixes in separate branch.

I wonder how the project is bit messy? It generates a project with all sub-targets where OTE is one of them. We use 3rd party libraries and want to keep them out from our code and cmake configuration so that's why it first looks bit strange.

CMake is a good way to do the eclipse configuration. I use it often, however I never compile from eclipse directly, instead of that I do it always via make on the build directory after the changes, but compiling should work straight away too in eclipse on linux where the toolchain is available in $PATH.

kortemik commented 8 years ago

@Yetta1 is it so that you have problems using eclipse? what kind of? I am more than happy to help you out with those.

Open Source drivers don't implement all the features required for OpenGL core profiles so they might not work, there are multiple issues on those in RBDoom3 repo, please refer to them in case you still have problems with your configuration.

Anthony-Gaudino commented 8 years ago

@Yetta1 I don't think that manually configuring xorg will help because as it says in Debian website FGLRX doesn't support EGL, and it's required by Gnome 3. I have the free drivers installed but it doesn't support my Radeon HD8000 series card.

I don't know how you tried using the Eclipse project but one way is this:

Git clone the project.
Add execution permission to build script chmod +x ./OpenTechBFG/scripts/cmake-eclipse-linux-profile.sh or chmod +x ./OpenTechBFG/scripts/cmake-eclipse-linux-debug.sh, you may want to edit those files to enable the use of SDL2 by changing -DSDL2=OFF to -DSDL2=ON.
Run one of the scripts ./OpenTechBFG/scripts/cmake-eclipse-linux-profile.sh, this will create a folder called build inside OpenTechBFG dir. The build dir contains the files .cproject and .project, those are the Eclipse project files that CMake generated.
Open Eclipse and click on File -> Import... open General clicking on plus sign (+) and select Existing Projects into Workspace, click Next. Click Browse on first option (Select root directory) and navigate to path OpenTechBFG/build, click Ok, on the Projects: list, there should be listed the OTE project and it should also be already checked, so click Finish and the project will be imported to Eclipse.

@kortemik I'm just asking because there is no option to limit line length set in astyle-options.ini. I will consider that there is no such limit then.

When I said that the project is a bit messy on Eclipse I meant that there's too many folders and files on it. In my project I see the folders and files in the same way a file explorer shows me and I didn't have to include all files/folders on it. For me there is no option to build the project created by CMake on Eclipse, on my project there is this option.

You can see the difference in the image bellow, on top there is the project I made by hand and in bottom the project CMake created. eclipse_projects

kortemik commented 8 years ago

Under [Subprojects]() there are the projects CMake is enabled to build. This includes their source code and build instructions and so on. All these are shown so that they can be edited in Eclipse.

On the root you see only the files that CMake knows as results, if you hit ^B (ctrl-B) it will compile the binaries to these directories. Objects for linking et cetera which will be used to debug the issues in case you run it with pressing F11.

Under [Targets]() you should find the most useful stuff but we haven't really put any effort to make it look nice in that way. The [Subprojects]() is handy enough and eclipse understands the relations between the source files so why to spend time on that.

Not that I'd say no if you really want to improve CMake configurations and spend time on it's grouping capabilities, I won't say no. It's not hard either, but something I don't see any point in, at least at the moment so I didn't fix those.

The way CMake is at the moment configured is that it has a parent project consisting of subprojects. The parent project(OpenTechEngine) doesn't actually have any targets itself. It just bundles up the subprojects to deliver the libs and the executable. That's why it doesn't look that clean either.

Tuning CMake for these is the right way. For MSVC we do have some grouping which works.

ghost commented 8 years ago

I have followed the steps for the Eclipse build on linux, however it seems that it searches for cmakelists in the home directory instead of OpenTechBFG, it also created a empty build folder in the home directory.

Using the Cmake gui gives an error that cegui/cegui.git does not contain CMakelists.txt

I changed the path, pointing to OpenTechBFG in the .sh file, it created an empty build folder in the scripts folder.

Anthony-Gaudino commented 8 years ago

@Yetta1 You don't need to change anything for it to work.

Now that OTE has the CEGUI subproject you must git clone this way: Lets say you want to clone to /home/yetta1/Documents, it will create a folder called OpenTechBFG inside Documents folder.

Open Linux terminal and enter: cd /home/yetta1/Documents
git clone https://github.com/OpenTechEngine/OpenTechBFG.git
Get inside OTE directory: cd OpenTechBFG
Since we have a git submodule (CEGUI.git) we must run this command git submodule init and then this command git submodule update. It will take a while to download CEGUI submodule.

Now Cmake should give no errors when you run the script.

If Cmake seems to be searching in wrong path, try running the cmake-eclipse-linux-profile.sh from the scripts folder: cd /home/yetta1/Documents/OpenTechBFG/scripts then run script ./cmake-eclipse-linux-profile.sh

DanielGibson commented 8 years ago

you can also just do git clone --recursive https://github.com/OpenTechEngine/OpenTechBFG.git and it should automatically get all submodules (i.e. CEGUI, we have no others).

ghost commented 8 years ago

I did git clone through the terminal like that when I got OTE. I have found that LinuxMint installs the SDL and SDL2 libraries to /usr/include instead of usr/local/include. It seems there is a difference in the system which is causing the problem.

Anthony-Gaudino commented 8 years ago

@Yetta1 can you please paste the messages that the terminal shows when you run the script. And if there is a file called CMakeError.log in the build dir please add it too.

I don't know much about CMake, but it should find where SDL is installed on it's own. There is a FindSDL2.cmake on OpenTechBFG/cmake/Modules though.

You can try changing this file, on the beginning there is:

SET(SDL2_SEARCH_PATHS
    ~/Library/Frameworks
    /Library/Frameworks
    /usr/local
    /usr
    /sw # Fink
    /opt/local # DarwinPorts
    /opt/csw # Blastwave
    /opt
)

try adding /usr/include on it.

ghost commented 8 years ago

@Anthony-Gaudino I removed the folder and restarted the clone by following your instructions, i never ran the git sbumodule commands, which may be why things aren't working. I still have to wait as my download speed is at 20kb/s :/.

As for AMD radeon drivers, try the ppa's for xorg-edgers and oibaf graphics drivers. The standard free drivers gave me 60fps on glxgears, after edgers and oibaf I got around 1400fps on glxgears by running it as DRI_PRIME=1 glxgears. Still no luck getting Doom3 to run, it's the linux version from idsoftwares repo.

Anthony-Gaudino commented 8 years ago

@Yetta1 You can use the git commands I gave or the git clone --recursive command given by @DanielGibson, this last one only works on Git version 1.6.5 or newer.

Thanks, I will take a look on that driver.

ghost commented 8 years ago

@Anthony-Gaudino CMake won't create the build directory in the terminal. Running CMake gui it generates however gives the following error message:

CMake Warning in CMakeLists.txt:
  The build directory is a subdirectory of the source directory.

  This is not supported well by Eclipse.  It is strongly recommended to use a
  build directory which is a sibling of the source directory.

Anthony-Gaudino commented 8 years ago

@Yetta1 This is not an error message, just a warning. You may want to check latter if you did something wrong while using terminal, but it seems that the GUI did the job.

Since CMake ran successfully, now all you have to do is import the project into Eclipse.

ghost commented 8 years ago

@Anthony-Gaudino It didn't create the .project and .cproject files, only 3 folders and a CMakeCache.txt.

Anthony-Gaudino commented 8 years ago

@Yetta1 Are you sure? Remember that in Linux, files starting with a period (.) are hidden files. On a terminal run ls -a ./ inside build dir to check if those files aren't there or change the settings of your file browser to show hidden files.

ghost commented 8 years ago

@Anthony-Gaudino I tried running it again, now the build directory is empty and also locked as root ownership, it also locked cmake_install.cmake

Edit: CMake says it has written files to build, however there is nothing there even when using ls -a ./.

edit2: i tried ls -a ./ on the OpenTechBFG folder, the .project and .cproject files were written in there.

Edit3: Opening the project from there seems to have worked, the project is building in Eclipe now.

Anthony-Gaudino commented 8 years ago

You probably ran then script as root.

Remove build folder using this command as root user: rm -r /home/yetta1/Documents/OpenTechBFG/build, change the path according to your system, be careful when using this command, make sure you write the path correctly or you may accidentally delete other data.

ghost commented 8 years ago

Seems like it is the case as I had to run the .sh file as sudo, otherwise it gives a permission denied message.

Anthony-Gaudino commented 8 years ago

@Yetta1 Yes, I suggest you remove the build folder and run the script as a normal user, not as root.

ghost commented 8 years ago

@Anthony-Gaudino Thank you for the help, really appreciate it. The build did however work and created the OpenTechEngine binary. Unfortunately I still have issues with the AMD drivers it seems, it's doing the same as Doom3, runs, then exits.

Anthony-Gaudino commented 8 years ago

@Yetta1 No problem, I'm happy I could help.

I don't know your computer specs, in my case the free drivers don't support my GPU and I would need to install the non free AMD driver (FGLRX), but then it would break my Gnome 3.

You said you installed the driver and you can run the glxgears, maybe you need to configure something for the game to work.

Also see what @kortemik said:

Open Source drivers don't implement all the features required for OpenGL core profiles so they might not work, there are multiple issues on those in RBDoom3 repo, please refer to them in case you still have problems with your configuration.

So maybe you have to install the non-free FGLRX driver.

I always install the FGLRX, only in this case I still didn't installed it because of the Gnome 3 issue.

DanielGibson commented 8 years ago

I suggest using XFCE, it sucks a lot less than gnome3.

when using fglrx, make sure the fglrx kernel module is actually used and not the open source radeon one - you should probably blacklist radeon, in case fglrx doesn't do that itself

ghost commented 8 years ago

@Anthony-Gaudino I don't think oibaf and edgers works with FGLRX, part of the guide was to uninstall it. I'm currently looking into it. It's an old HD3200 card on an old AMD laptop, my other rig uses Nvidia. I'm still looking into my current issue, downloading nexuiz classic to see if that works. You could try following the steps on this link, it includes info for new and old cards:

http://www.pcworld.com/article/2025909/gaming-on-linux-a-guide-for-sane-people-with-limited-patience.html

ghost commented 8 years ago

I'm thinking of switching from cinnamon to the Awesome window manager, seems very light-weight on resources.

kortemik commented 8 years ago

I think this should be continued on our discussion board rather than here.

OpenTechEngine / OpenTechBFG

Code formatting #71