Messages

31

Programming / Re: How to be a horrible programmer: OpenXcom coding guidelines

« on: September 25, 2013, 12:58:01 am »

I've read the document, and here are my comments:

C++ Guideline 3.1.13 Verify that all classes provide a minimal standard interface against a checklist comprising: a default constructor; a copy constructor; a copy assignment operator and a destructor.

"ALL" - this isn't ok. boost has it's noncopyable class for a reason, and this reason is that so much classes weren't meant to be copied at all.

C++ Guideline 3.2.4 An abstract class shall have no public constructors.

this is a purely useless rule.

Use public derivation only.

disables you from layering code common for each object instantiated for a template, which makes your compile generate tons of redundant code.

C++ Rule 3.3.5 Override all overloads of a base class virtual function.

if you really need  to do this because of "safety", then your classes need a redesign.

Rule 3.4.5 When publicly deriving from a base class, the base class should be abstract.

in such cases it's always a composition vs inheritance issue to talk through in the team and evaluate the profit of each solution, sometimes inheritance wins, so the rule isn't always viable, and by that shouldn't be called a rule.

Rule 3.5.4 Make binary operators non-members to allow implicit conversions of the left hand operand.

not always what you'd like to have actually...

Rule 5.2 For boolean expressions ('if', 'for', 'while', 'do' and the first operand of the ternary operator '?:') involving non-boolean values, always use an explicit test of equality or non-equality.

in case of 'if' and 'while' this is actually a very redundant rule.

C++ Rule 7.1 Always use casting forms: 'static_cast', 'const_cast', 'dynamic_cast' and 'reinterpret_cast' or explicit constructor call.  Do not use any other form.

this is actually CRAZY IMPORTANT. using these guys is not only safer and nicer, but it's a nice and free tool to make first general verification on the code quality.

C++ Guideline 10.6 When comparing variables and constants for equality always place the constant on the left hand side.

bad guideline, already explained why.

C++ Rule 12.7 Document that operator new and operator delete are static by declaring them static.

this is just silly...

Rule 15.1 Do not use variant structures (unions).

they are required to make a nice memory pool, and not die in the process but in general I agree with this.

Rule 16.3 Only instantiate templates with template arguments which fulfill the interface requirements of the 'template'.

disagree - if all you need from a template is the part of functionality that your object fulfills, then it's better to use it that way, than to write code without reason.

Guideline 16.4 Only use templates when the behaviour of the class or function template is completely independent of the type of object to which it is applied.

true, but only in cases when the template is for everyone to use. making private template methods that will handle a handful of types, to save a lot of code is ok, and it IS a way to go, rather than to duplicate the code.

Rule 17.8 Never create containers of auto_ptrs.

CRAZY IMPORTANT

32

Suggestions / Re: a thought from a c++ software developper

« on: September 24, 2013, 11:32:29 pm »

Interesting proposition. You should say something about my favorite C++ feature: template metaprograming

I don't think that there will be that much room for really interesting things on meta programming or using SFINAE in this particular project, but when I see a suitable place I will gladly propose something. Most of all there aren't any performance problems caused by things like extensive usage of trigonometrics or other slow functions like this that would require a compile time computation on taylor series for some number of elemnts.

Not making generic purpose stuff I don't see a place for SFINAE here either making some shenanigans with conditional template compilation is fun, but here would have been achieved by polymorphism and an interface and at the same time be a lot more readable.

static polymorphism - well maybe, why not - at the first thought. but on the second - data will be there either as references or pointers either way, and sparing one assembler command per a method call isn't enough to sacrifice the readibility in my opinion.

I do LOVE templates, but only when they are used properly, and when they are done by layering if possible ( and often it is ).

But I would also like to see your suggestions applied directly to the code

if I will just fix and commint, noone will ever notice until something really major gets changed. noone will learn from that I think. and if someone reads the intentions of the change wrong - well that is even worse. my proposition here was

33

Open Feedback / Re: Promotions

« on: September 24, 2013, 04:45:12 pm »

what bugs me in the topic of promotions is that I had guys who went on maybe just a few missions, and then were sitting in the quarters doing totally nothing for months, and they were getting promotions.

34

Open Feedback / Re: X-Com reaction fire mechanics

« on: September 24, 2013, 04:42:38 pm »

(now, if you want to talk messy, convoluted and redundant ( welcome to c++ ) that's a different cake)

this isn't a thing of c++, it is a thing of bad coders. I have seen better written and more readable assembly code, than java. it is not a problem of any language - it is always a problem of how the programmer is able to express their thoughts and the process of coding. there is no (and will never be any ) language on earth for example that will make a thought process put into file look good when it is by the algorithm : "ok, so I will change/add this fully or semi random thing, and will check if it works..."

35

Suggestions / Re: a thought from a c++ software developper

« on: September 24, 2013, 03:48:34 pm »

Perhaps I could make a topic in the development section (or even a full section with independent topics?) with something like tutorials about some concepts ? with examples from the codebase ? that might come out fruitful in the end. What do you guys think ? how many recipients would such a section have?

I could cover topics like encapsulation, how to really do that, why is it there for, what are the scopes of encapsulation etc, how not to mistake object based programming with object oriented programming, stuff about testing software, tools for testing, how to make your software more testable, more flexible for changes, and how to desing(or refactor) stuff into things, that can be extended, but won't need modification and so on.

anyone intersted say "I!" in this topic

(I'm sick and feel barely alive now, that is why I am inactive lately )

36

Troubleshooting / Re: Elerium issue

« on: September 24, 2013, 03:31:52 pm »

Maybe you just mistaken intact elerium for intact ufo power source? just seeing the power source core doesn't mean there is elerium in it, and YES it can be disginguished by looking at the power core: if at the base of the power source you see three dots, then there is not elerium in it. If you see those three dots covered by something that looks like a mindprobe/grenade, then there is 50 elerium in that core.
have you checked that?

37

Troubleshooting / interesting bug in battlescape - load/save situation

« on: September 23, 2013, 10:13:08 pm »

ok, so here it is, done out of pure curiosity:

1. my soldier has gone berserk
2. I spammed the options tab, and clicked save while he was in this mode
3. I load the game, and the guy is all stuck with his gun pointed in the ether.

If I try to move him, he the game will hang, and will be have to killed with process manager.

I attach the save game.
EDIT:
if anyone gets to this, I have found a workaround: order the guy to shot in front of him. he will make a shot, take his weapon to 'stand by', and will be mobile again, and wont hang the game.

38

Troubleshooting / Re: bug - using the "Sack" button on the soldiers view

« on: September 23, 2013, 09:24:31 pm »

I have this problem on build 2013_09_16_0025

39

Troubleshooting / Re: bug - using the "Sack" button on the soldiers view

« on: September 21, 2013, 04:07:16 pm »

I don't have a save, but it happens like every time... it's very annoying.
as to the bug tracker - it's not convenient for me, because there's no SSO here : p

40

Programming / Re: How to be a horrible programmer: OpenXcom coding guidelines

« on: September 20, 2013, 10:28:34 pm »

I would do "int x, y, width, height;". The grouping implies they make sense close together

then why not just making a class with all public fields named Rectangle ? the class that used those coordinates and dimensions would also have thinner code, and be easier to maintain/debug and this wouldn't affect the performance at all, you only would have to place it as a value field(not a pointer or reference) and propose that it's methods would be inline - NO overhead at all

it could've also scared away everyone that doesn't have a clue what I'm on about.

That would actually be a very positive thing if I am to be honest

if/else nests are more error prone and out of experience I know that people do a lot worse in chaos - brain works perfectly when it can categorize everything and comprehend the processes in terms of categories. That is why code like:

for( Agent& agent : agents )
{
    agent.Update(timeDelta);
    agent.HandleMessages(messages);
    agent.ComputeAction();
}

with classes RangedAgent and MeleeAgent is far superior to code that would do the same, but have if/else nests (or hives). if you want to correct RangedAgent -> you go to the class to a separate and categorized environment. You change 3 lines of code, et viola! done. if/else -> track and change all the 20 places in code and pray you didn't brake the existing logic ( even the conceptual flow of updating, handling messages and computing the actions, because your changes are not external to this simple algorithm, and might have changed it in case of if/else hive ).

Game concepts rarely fit nicely into "polymorphic objects"

That is false, sir.
Game concepts are as abstract as any other, and there are exactly the same rules, same constraints. making tall trees of inheritance is as bad and crippling in any case of programming as it is in games. just keep your hierarchies flat, and you'll stay safe, clean and effective. when games are compiled they is always a speed optimisation vs size optimization and it is not that rare, that size optimizations win. if/else rich code loses in that matter vs polimorphic calls in a well designed engine.

a concept that isn't working with polymorphism are any kind of optimized memory allocators, but as I've seen so far, you don't have any in the project.

41

Troubleshooting / bug - using the "Sack" button on the soldiers view

« on: September 20, 2013, 09:13:44 pm »

SupSuper, you've said you're the geoscape developer, so here is something for you.

When you go base->soldiers and pick a soldier, then you can sack him using the Sack button in the upper right corner.

If your currently viewed soldier is the last one on the list and you click the sack button, the first solider on the list gets deleted as well.

42

Programming / Re: A bug in battlescape and a fix for it

« on: September 20, 2013, 07:47:19 pm »

If you want a nicer global random number generator use mersenne twister from the <random.h> , there are two versions of it in the header. You are using VS2010, so you should have that header. an implementation of mersenne twister is also easy to find on the internet if you can't include this header.

I have looked into that issue in terms of bug/not bug according to what I have read here on the forum, hence I analysed and found a solution. I have called it an "anti cheating mechanism" because I haven't found better words for that. I, in general, don't consider bug-using as cheating. Whether it is a bug or not - this is a feature of the game.

the seed is to make bugs easy to reproduce

Well, if the bug is related to anything generated by the RNG ( and from that comment I guess that is the case at least from time to time ) then it is easy for the user to forget about something that would seem to him an unrelated detail. Like, for example, shootting the wall in the scenario I've given you before. Good luck with deducing what made the sequence of random numbers in the user's case other than the case of your bug reproduction. Because of that you can never actually know if the bug is not reproduced because it is not there, or if it was not reproduced, because you didn't do exactly the same thing that the user did

Seed per unit is no overkill, to be honest, it's just 16 bytes if you get a small generator ( 4 for the dirty fix ), which is very little when you take into account the scale of a battlescape instance - no more than 50 units on the map.

real time commercial games have a random generator per thread. those which want to have a "replay" functionality, use saving the seed, they use a single random generator for mechanics ( game mechanics are "single threaded", although the game as a whole uses multithreading ) and a constant update step for the game state. the model is different in that case, because the time flow is constant. there is also no chance of one unit making all of his moves before every other unit ( like here - utilizing the whole pool of time units ) and also the majority of games have a very little use of random number, like in starcraft:broodwar it was used only in case of attacks vs units on upper ground and behind obstacles which was really rare. here it is a key concept

43

Programming / A bug in battlescape and a fix for it

« on: September 20, 2013, 02:22:03 am »

Hello again!

SupSuper or Warboy, could you please take a look on that?

I remember that I've read hear on the forum, that there is no reason to try and load the state of the battlescape and try to save units, because you save the seed. I've taken a look into the code, because I was wandering what random generator are you using ( would it be a mersenne twister, or some simple noncryptographic randomizer ) and found out that you are using a global seed (at least for the battlescape). I immedieately have found a way to trick your anti cheating security done with saving the seed.

Scenario 1:
1) alien, and two of my soldiers see it, my turn
2) I save the game.
3) I shot with guy1. I miss. Alien retaliates. My guy dies.
4) I load. I shot with guy1. I miss. Alien retaliates. My guy dies.
5) I load. I take guy3, that is somewhere far away. I shot a random wall.
6) I shot with guy1. I miss. Alien retaliates. ALIEN MISSSES

Why this happens in detail?
1) Lets say my seed is A, and because of that the sequence of numbers generated is B,C,D,E,F,G,...
2) You have saved a global seed, so no matter who uses the random number, the sequence goes forward.
3) in scenario 1, right after I save the game, guy1 gets value B from the RNG and he misses. Alien gets value C, and hits.
then I load the game, so the seed is again A, guy1 gets value B from the RNG and he misses (again) and the alien gets C, so he hits.
then I load the game, so the seed is again A, guy3 shoots the wall getting B from RNG, guy1 gets C and he misses, but alien gets D (no juvenile humor here!) and misses.

I have came up with multiple scenarios of this, and the only way to handle this properly is to keep an instance of a random number generator in each of the battle units.

Let's take the same scenario, with a random number generator instance per a battle unit, shall we?

alien has seed A, guy1 has seed G and guy3 has seed M.

Scenario 1 with an instance of a random generator per a battle unit.
1) alien, and two of my soldiers see it, my turn
2) I save the game. ( with all the different seeds for each unit )
3) I shot with guy1(he has A, so his generator generates B). I miss. Alien retaliates (it has seed G, so he gets number H, and hits). My guy dies.
4) I load. I shot with guy1(he has A, so his generator generates B). I miss. Alien retaliates (it has seed G, so he gets number H, and hits). My guy dies.
5) I load. I take guy3, that is somewhere far away. I shot a random wall(he has seed M, hence getting number N and not changing the next value that the alien and guy1 will get).
6) I shot with guy1(I loaded, so his seed is A, and he gets B from RNG, because he has his own seed). I miss. Alien retaliates(he has his own seed G so he gets H again and hits! ). my guy dies

a lazy fix would just be to have an int in each battle unit named _seed and get his test values by a method like:

Code: [Select]

int BattleUnit::GenerateOwnRandomNumber()const
{
srand(_seed);
_seed = rand();
return _seed;
}
but I would advise using an object, because this method is "a bit" slow. You can try something from <random> header (c++11), or if you wait a day or two I will get you a nice, and small piece of code ( compact in size and fast ) random generator, that isn't cryptographic, but had nice results... I just need to find that implementation on my email

Regards,
djemon_lda

44

Programming / Re: How to be a horrible programmer: OpenXcom coding guidelines

« on: September 18, 2013, 08:39:07 pm »

@Fenyő:
I don't agree with your postulate. what if you compare two variables? doesn't change virtually nothing in that case if you make the ==/= mistake, but another thing WILL happen. If a programmer get's used to this rvalue on the leftside idiom, then he will be careless about it and will incorporate errors with ==/= when comparing two variables. this is a poisonous gift because of that. I personally haven't made such a mistake on any point - EVEN when I was porting from pascal to c++ on university ( not that in pascal, you used a single = for comparison, so I should have a grander potential to make this type of error, yet I never did - just because I was paying attention to my work ).

@SupSuper:
I have some suggestions if you'd like to consider them:

Use comments to explain long boring code blocks.

Why not considering to extract a method with a meaningful name in such situation? Such a method is easier to test and easier to debug, and extracting methods allows later on to see if "doing this is stil a responsibility of this class, or maybe we need a new one?" Also inlining code is more optimal only in certain situations, overall it is ok for performance to make a function call, because when bloating code, you put a lot of stuff into the processors 1st line cache, hence having less place in the 1L cache for the game's data, and increasing the cache misses rate, which in turn are very bad for performance.

Feel free to put multiple declarations in one line.

When declaring pointers, keep the * on the side of the variable name. This prevents common mistakes like "int* a, b, c" (only a will be a pointer).

I disagree with the first one, and the reasoning I have nullifies the second one. I propose to make it: "Never put multiple declarations. Each declaration has it's own line".

It is easier to read:

Code: [Select]

int a;
int* b;
int c;
int *d;
than:

Code: [Select]

int a,*b,c,*d;
The reasoning behind int* a; being better than int *a; is that int* gives a stronger intuition of a type - pointers are also distinct types after all, and you can also make references to pointers like int*& a; (which in turn are types as well) or pointers to pointers as well.

Another thing is padding. Making many fields make's you lose count easily - if you have five bools in a row, and then an int then you expose your classes for problems with padding memory -> objects take more memory than it actually is required -> you get more cache taken, but not usable -> performance goes down.

Use pre-increment/decrement (++i and --i) instead of post-increment/decrement (i++ and i--) except when it produces a different result.

Assembly code generated for ++i and --i for an int/long/char has equal complexity ( both generate 3 assembler instructions ) - this is valid only if the operator pre/post incrementation/decrementation is defined for big objects, but it is discouraged. Overloading of the ++ or -- for types other than BigInts etc is a bad practice and destroys readibility of the code.

Avoid making generic utility classes or functions, keep everything in the class it's relevant to (make more classes if you have to, can never have enough!).

In general it is a good practice to make generic stuff. If you make a private method, then it is worth considering if this functionality could be use for other classes.
If the answer is 'yes' it's good extract that method to another class as a static method (perhaps templated) and reuse the code. Less code == less places you need to maintain, test and track bugs in.

Use common sense!

It isn't worth to overestimate this

What I'd add is also to try to use polimorphic classes, to reduce the hordes of if/else statements and switches. The generated assembler code for a polimorphic method call is just a single assembler instruction more, which makes it more efficient than using if/else/switch because there is also no forking and branch prediction incorporated.

Btw, were you guys doing code reviews before commiting the changes to the repository ? it's worth doing, believe me.

45

Programming / Re: Features: Show chance to hitting and the throw trajectory

« on: September 18, 2013, 05:30:21 pm »

perhaps you could make some kind of ray tracing to determine the precise area of the granade blast ? with the area described by the shape (not necessarily a circle then) the precise area including stuff stopped by the walls etc ?