Janitorial work

Post by **safemode** » Thu May 31, 2007 11:42 pm

Having success with std::list in UnitCollection/UnitIterator. I've decided it's time to move on to my original goal of cleaning up the headers and source files from namespace pollution, unnecessary header includes, code in headers rather than source files, flat out errors, and lastly, but probably most annoying of all, formatting.

I'm going to be patching the files under the following rules I set for myself.

1. No "using" directives in headers, at all.
2. No implementation in headers except in const func const; functions and templates.
3. Headers only include headers required for the code in themselves, implementation includes all headers required for the code in itself.
4. No "using namespace std" allowed at all. We only use individual data types.

Formatting:
1. No space indents, all indents are tabs.
2. Braces on new line of functions and classes, same line for procedural blocks.

That's as much as i get spelled out as i can think of right now. Any additional changes i make are probably too minor in detail to be mentioned.

First up is unit_generic.cpp/.h

Post by **safemode** » Fri Jun 01, 2007 2:23 am

I'm 1/3rd of the way through unit_generic and I felt compelled to make a few observations.

First, unit_generic is the result of probably years of hacks, and understandably, it's organized fairly badly. That's easily fixable.

Second, unit_generic contains information about all the specific units that are supposed to be derived from it. What's the point of derived units then? We're destroying 90% of the whole purpose of inheritance and abstraction layers. What class in the game isn't a friend of Unit?

third, unit_generic has far too many public members. Sometimes the lack of layers is blatent. SubUnits is right above getSubUnits and viewSubUnits. If I can access SubUnits directly, why do I even need getSubUnits and viewSubUnits?

I dont want to get too far ahead of myself right now as my goal is to clean up right now, not rewrite. But I really dont think unit_generic is going to be able to remain in the state it's in. It completely destroys any hope of layering and abstraction in the entire program.

This biggest change in unit_generic that i'm going to do right now besides simple formatting and cleanup changes is making the virtual functions that are empty to what they should be, fully virtual functions. Any class that inherits unit will have to define them, either empty or with code...as it should be.

charlieg · Post by **charlieg** » Fri Jun 01, 2007 9:54 am

Good stuff

Lokysan · Post by **Lokysan** » Fri Jun 01, 2007 10:38 am

I was thinking about maybe trying to help with the development, but as I don't have much experience in game development I was not sure if I could really help that much without first spending lots of time just reading the code and guessing how it works... but after reading your posts, I realized that I could actually do something almost from the start even if it is just "janitorial work", and at the same time learn more about the vegastrike internals so I could later do a more "interesting" contribution.

I will update to the latest svn next monday (this weekend i'll be busy) and start having a look at the code, so expect a message from me soon asking what are you doing and where I could be of help...

Continue with the good work!

Lokysan

Post by **safemode** » Sat Jun 02, 2007 2:04 am

I'm about 3/4 of the way done formatting and taking the implementation out of unit_generic.h and putting it all in .cpp. I'm also changing any structs that define methods and turning them into classes. I know structs and classes are the same thing in C++ cept default access, but I still believe structs should be "methodless" constructs.

After i'm done this pass, I'd like to try and re-group the methods and variables a bit. Turning most into private/protected and only un-protecting those that are actually used publicly for now.

Eventually though, i really think any vegastrike developers out there should get together and we should really look into redoing Unit. It's complete garbage, everyone knows it, and it's really high time it's redone the right way (tm).

Basically, we have the derived types that we need, we just completely ignore layering. I'm proposing we enact strict layering. I really dont think it'll be that hard to do once unit_generic.h is readable. Some python/other code will be affected slightly, casts will need to be made. That shouldn't be bad at all though, because any code using unit-type specific variables/functions, already knows what type of unit it's dealing with, so casts should be no problem at all.

The only change in derived types we may want to look into is dealing with physics of motion. Guns and turrets and other upgrades dont need all that, but ships, escape pods, bases and planets and asteroids do. Perhaps we have Units -> Newtonian. Newtonian is simply a derived class of Units with all the physics functions and variables. All ships and objects that move in space, inherit Newtonian, all upgrades and weapons (not missiles) inherit Unit directly.

On second thought, I think what i'm coming to is that templates seem to be hurting us. Greatly. Much more so than they're worth in their current use.

I'd be very interested to hear any arguments for keeping templates in Units, since it seems obvious that it doesn't lend itself to generic programming. At least not at the Unit level, and i doubt it's usefullness at any other level dealing with Units.

Post by **safemode** » Sat Jun 02, 2007 5:06 pm

Having finished formatting and making some minor changes to unit_generic.h/.cpp it's seriously a miracle that this program compiles and works at all. And all the complexity fully falls back on how Unit requires everything. I'm going to need more time though to see how we use templates and how/if templates can be used if we truly layer out Unit the way it should be.

Post by **hellcatv** » Sat Jun 02, 2007 6:43 pm

This is really amazing work safemode...I'm going to have to go over your notes in the other threads in the next few days...

one of the mods is doing a release in the next week or two, so I suggest we wait until then before applying this to anything but a branch (theres no way we have time to track down any bugs that could occur from typos in that time) but as soon as that crew gets their code out we'll apply your stuff to the main trunk! (and since there are only bugfixes from here on out I doubt it will be difficult to merge in your stuff)

do you have developer access yet, or have you been using patches to send over your ideas? because we could start getting them in there by making a new branch straight away

Post by **safemode** » Sat Jun 02, 2007 7:32 pm

It appears I'll have to completely redo the work i did in unit_generic.h/.cpp i'm getting segfaults and I really shouldn't be.

At this point i'm wondering if reformatting and cleaning up unit_generic is even worth it. I really want to destroy it and create a new layered Unit setup that makes sense. Hrm.. I'm going to have to think about this, I dont really know enough of how units in game are created and used via python and the rest of the source to know where to actually start.

This would be a major rewrite and perhaps it's best if i skip it and move on with the cleaning. The problem is that unit_generic is polluting so much code. I mean we might as well prototype every single class in the entire program and put it in vs_globals.h and include that in every header to start. Anyways, i'll see where this takes me. Perhaps someone else is ready to fix it too.

For now though, i'm going to continue to try and find out why i'm getting segfaults. I do intend to keep everything for now as bugfix/cleanup oriented as possible. No real changes at the moment.

Post by **safemode** » Sun Jun 03, 2007 4:34 am

Well, I got sidetracked a bit and made an update to UnitCollection (check out other thread). I'm currently back to 50% done re-formatting and de-polluting unit_generic (i'm back at line 600). I'm tearing functions out of it a little at a time and recompiling and seeing if i get the same segfault i got before. So far so good.

Namespace pollution wont be fixed until it's fixed everywhere though. Too many files require including too many headers (because layering is just completely ignored). So after unit_generic is done, i'm going to do a search for "using" in all headers and remove them all as part of a separate patch.

Should be done tomorrow (both patches).

Post by **safemode** » Sun Jun 03, 2007 5:55 pm

Well, after hand-indenting and all that I found that my editor had an option set that was tricking me into thinking i was indenting when i really wasn't. So i fixed that and decided to use bcpp to fix things.

There is more to the following patch than just indenting and bracing and such. I removed the using statements, fixed the use of STL classes to use the std namespace, as well as XMLSupport. I removed all the functions that aren't "func const {}". I didn't want to do much more than that right now since this is supposed to be cleanup only.

I also grepped through unit_generic.cpp looking for postfix ++ operator usage when prefix would be better. I came across this line that seems suspicious.

around line 7393 (after patch )
AddToDowngradeMap (up->name,1,curdowngrademapoffset++,tempdownmap);

Does AddToDowngradeMap want the current offset in that function, or the next value of curdowngrademapoffset? because postfix ++ returns the current value, not the value it's incrementing to.

I left it as is for now because i'm not sure what that does.

here's the patch. Unfortunately, keeping 5 different branches locally to keep my various "projects" in line isn't going well. I had to roll up the cleanup patches with my stdlist unitcollection patches, since stdlist hits unit_generic.h (small inline function removed).

This should be a fairly topical patchset despite hitting a bunch of files. If i were an official developer, i'd still suggest this patchset for inclusion into the next release. Hell, I'd even put it in the current release. It's more stable than keeping track of your own linked list class.

Rolled Up unitcollection/cleanup patch

I'm using bcpp with a slightly less than default configuration file to indent the files. Then i go through it and hand-edit anything that needs to be changed. If anyone has a problem with this speak up so i can figure out what to do about it.

Post by **safemode** » Sun Jun 03, 2007 6:54 pm

BTW, if someone really wants to separate the two patches if one is to be included in svn and the other not(yet) here's the stdlist only patch.

stlist_unitcollection_6-3-07.patch
The only thing it doesn't do is delete src/cmd/iterator.h I haven't yet figured out how to get svn diff to notice deleted files. it's totally unused now so it should be deleted.

Here's the cleanup patch to unit_generic. SHOULD be applied after the stdlist_unitcollection patch.

unit_generic_cleanup_6-3-07.patch

I'd like to get this submitted before my changes get even larger and harder to separate out. However, further patches should be simply incremental, and not touch other files in a non-related manner.

Post by **safemode** » Wed Jun 06, 2007 12:19 am

I take the lack of response as at least, nobody's compile borked out of control when they tried the patches. So i'll continue on my formatting changes. Starting in ABC order in src/cmd first.

Each pair of files will be a separate patch that should be independent of eachother. Meaning, that besides the stdlist_unitcollection patch, you shouldn't need to apply any of my other patches to have a functioning compile of vegastrike.

I realize how much developers hate touching all the files with patches, but since this is a relatively slow period of development, there really is no better time to reformat the code so it's consistent.

Lokysan · Post by **Lokysan** » Wed Jun 06, 2007 9:50 am

Hi! I've been "playing" with the code last two days, but I didn't have the time for testing your patches, I'll try them as soon as I can. The bad thing is that this week I'm quite busy, so I will not be able to do anything useful until next week... anyway, continue with the good work!

targ collective · Post by **targ collective** » Wed Jun 06, 2007 3:53 pm

I'd happily test it, but I've no idea what it does or is for.

If you're posting a release, you should explain what difference it will make to gameplay and why it's important, otherwise people are unlikely to take the time to download it.

Please forgive my ignorance, but I wouldn't know an STD:List if it bit me, wearing a pink tutu and doing the cancan.

...

So, sorry to be tiresome, but what exactly is it you're doing in layman's terms?

mortaneous · Post by **mortaneous** » Wed Jun 06, 2007 4:12 pm

Std:List is back-end work for efficiency in handling data... it shouldn't do anything to the gameplay if it's done correctly (well, maybe a little less time lagging, but nothing easily noticeable I'd imagine)

The rest is code cleanup which will make further dev efforts a little easier, again, gameplay shouldn't change if it's done right.

targ collective · Post by **targ collective** » Wed Jun 06, 2007 5:51 pm

So if you don't understand it, you probably don't need it. Okay then, fair enough.

Post by **safemode** » Wed Jun 06, 2007 11:31 pm

None of the changes i've made change gameplay. They're all backend and they're all "Cleanup" type fixes. The only people interested in them are other developers...and that is all i'm directing my threads to. The only reason why it's on this forum is because only monitors can post to the developer's focus forum.

The things I want people to look for are new bugs caused after the patches, and the good aspects, smaller compiled binary, maybe smaller memory footprint, faster compile times, more readable/correct code.

Post by **safemode** » Thu Jun 07, 2007 3:09 am

Just wanted to do a little update on the bugs i've come across.

I've come across numerous uninitialized variable uses. Functions will declare a variable but not give it a value, then use it assuming it's value is either set in some conditional (that may not be executed) or be 0. Now that i think about it, i haven't checked that all variables in a class are initialized to some expected value in the constructor (if they need to be initialized there). Should be fun.

Another bug i found so far is in base_interface.cpp, here's a snippet of code: You should find it somewhere around line 732

if (state==WS_MOUSE_UP&&links.size()) {
int count=0;
while (count++ < links.size()) {

This is an overrun. count++ returns a copy of count, then increments. That means count will == links.size() on the last iteration of the loop, and that will mean the loop will iterate links.size() + 1 times, since it starts at 0. I changed it to ++count. That should fix the problem.

Right below it i see (like my previous post's questionable postfix increment)
Link *curlink=links[base->curlinkindex++%links.size()];

This modulus's curlinkindex and links.size(). curlinkindex is incremented after the expression is evaluated, since the expression actually uses a copy of curlinkindex's current value at that time. It's almost certain that this is not the intended operation.

I've yet to hear anything about the last questionable postfix increment use, so like that one, I will leave this one as is. I suspect it's a bug though.

Around line 998 you'll find a line like this:
while (( mission->msgcenter->last(i++,last,who))) {

same thing, i is passed to last(), not i+1. Is that the intended behavior? It could be since the way it seems to be used makes sense. It kind of corresponds to for(int i = 0; mission->msgcenter->last(i,last,who); ++i){

Anyway, the cleanup goes on.

targ collective · Post by **targ collective** » Thu Jun 07, 2007 1:52 pm

You really are capable, Safemode. (At least, you've lost me...) When you've got this cleanup business done, could we Privateer Universe folks recruit you? We could really use someone who knows their way around the .exe right now - Chuck Starchaser is looking into that, true, but he's having to do things by feel a bit (he's great, he just doesn't quit no matter the challenge) and we could really use expert help.

Post by **safemode** » Fri Jun 08, 2007 9:31 pm

I'm doing work right now that is pretty mod-agnostic. It helps everyone. I'm not working on gameplay stuff anytime soon. I want to clean up stuff and work out bugs and then focus on fixing the Unit class. I think it's seriously disgusting.

Post by **safemode** » Sat Jun 09, 2007 5:35 am

msgcenter.cpp in src/cmd/script has a function called last in msgcenter.

Last is usually called like this:
while((mission->msgcenter->last(i++, last, who))) {
picker->addCell(new SimplePickerCell(last.message));
}

i is started out as 0. Like i've mentioned before i++ makes a copy and returns that and then increments. This led to code in the last function to be as follows:

bool MessageCenter::last(unsigned int n, gameMessage & m, const std::vector <std
if (who.empty()&&whoNOT.empty()) {
int size=messages.size();

int index=size-1-n;
if(index>=0){
m=messages[index];
return true;

Now, the correction for the postfix++ call is the size - 1 - n. With prefix++ we would reduce that to size - n; That saves an operation in last() and a copy in all calling functions. I plan on making this change with my cleanup patches. Which are churning along btw. I'll make an initial release of the cleanups soon.

Post by **safemode** » Sun Jun 10, 2007 1:21 am

A couple calls to last dont increment the value as they're called. I'm not sure if they made assumptions about last or if they were incorrectly calling last the entire time. In any case, i've run vegastrike with my change to last() and saw no inconsistencies in the info / news menus of the base computer or ship's menus. So i'm going ahead with the change.

i'm going to post the files i've processed so far. They're not completely cleaned by any means. And there are changes to other source files in the build tree that aren't included in these patches. These are just the files that i've formatted _and_ attempted to clean up.

If you apply these patches, they will likely cause bad things to happen to your vegastrike binary. There are bugfixes in other functions that these patches assume to be in your bulid tree that wont be.

So anyways, for those that want to see what i'm up to. Patch these and check out the resulting files. Anyone with disagreements as to the format of the code speak up. we can discuss and i can alter my script.

asteroid_cleanup
asteroid generic cleanup
atmosphere cleanup
atmosphere server cleanup
base cleanup
basecomputer cleanup
unit generic cleanup

These can be applied individually in any order. They should be applied after the stdlist_collection patches. They should not be applied and built however, for reasons stated above.

anyways, work continues. i'm starting a new job soon so i may have trouble finding time but I do intend to continue reformatting and cleaning up all the files. Expect this run of patches to be very cursory, meaning, the bugfixes and cleanups are going to be mostly non-invasive and "simple". once i go through everything, i'll be able to go back and make deeper fixes as i see them. Hopefully by then though we can setup a specific goal and roadmap to the next release that everyone can focus on. Also, i think it's kind of sad that it's been 8 months since even an update on the homepage.

Post by **safemode** » Mon Jun 11, 2007 12:42 am

Sun bug :

While testing the latest changes i've been making, i've noticed that when the Sun in a system is targeted, the corona of the sun looks nicely blended and the effect is good. when it's not targeted, it looks like a planet with the corona distinctly separate from it.

This is not due to my changes. I have no intention of fixing this bug anytime soon, was just mentioning it if anyone else is working on it or experiencing it.

Post by **safemode** » Mon Jun 11, 2007 1:23 am

Another gameplay bug i noticed when testing is that you can't shoot your guns when going diagonal with the keyboard in any direction but down right. All other directions cause the gun to stop responding. And, if you are shooting and then decide to move diagonally, the ship wont move diagonally in any direction but down right.

Oh, and one more thing. I collided with the caine B sun at SPEC speeds. Segfault.

charlieg · Post by **charlieg** » Mon Jun 11, 2007 3:46 pm

The keyboard bug could be to do with keyboards not being able to work with more than a few concurrently pressed keys. This is definitely true on older hardware although I'm not sure whether more recent hardware works better when pressing multiple keys at once.

For a fact, my 7 year old machine with a ps/2 keyboard could not handle more than 2 or 3 concurrently pressed keys (2 or 3 depending on the combination of keys) which prevented me from playing multiplayer games with my brother using the same keyboard.