With cordite in the air, splintered steel, shell casings and powder burns, there’s only one explanation...
Post feedback & meet new friends. General SG discussion.

Moderators: Pardner, TheDoctor

Big 1.1 source code review (closed)

Postby Lucky Bro » Wed Aug 19, 2009 5:10 pm

Good day everyone!

I want to ask everyone here to review the new upcoming code.
Current revision now is 263.
I think main developers may pause merging to main branch for that review time.
Also I hope someone of moderators will create a separate thread on code section on forum with name like "1.1 code review" or "1.1 Bug fix".

Why do we need this?
Some of us think that Tequila can do the job by himself all alone and we'll get good working code to new year holidays.
But just imagine what he really does - he need to take care of all 1.0 features to add ioQuake ones and not broke anything.
ioQuake has a lot of improvements compare to vanilla Q3.
Smoking guns 1.0 has not less than ioQ3.
It is a huge work by many peoples.
And therefore the mistake probability is very high.
If you really like the game you may took some responsibility for it.
Anyway I'm not insist if you don't agree with me - just ignore the thread.
What do I need to know if I want to review?
Basic knowledge of C and logic :)
Without C it will be very hard to you, but still you may participate if you have wish.
How do I perform review?
You have to have three sources:
1.0 source code from SVN
1.1 source code from SVN
ioQuake source code
You have to compare equal files and check the logic. Do best as you can. If you don't know what this or that function does or don't understand - don't hesitate to ask (better on IRC channel first). But be sure that almost no one knows whole code, so most of times you'll have to check it by yourself.
I found a bug/collision/missed function/redundant piece of code!
Great. Half of job is done.
Now you have to report it at code subforum in newly created section.
Just create a topic, do a basic explanation what you have found.
If you can - provide a patch how to solve the trouble. If not - suggest a method how to solve. If you don't have clue how to fix it - better to recheck your bug - maybe it is not a bug? Still if nothing helps - just do a report.
What do I get for this?
Nothing.
What are those SVN, IRC, ioQuake3 backport?
Use the search on this forum to find out.
I don't want to and I don't think it is needed, lets save some trouble
No problem. I'm not insisting. Thanks for attention.
When my bug will be fixed?
Will be written in you thread.
I think bugzilla is better than forum?
Me too. Have you found any bugs?
So, there is not many who want to do it, so lets use forum. Also there are a lot of guys who doesn't know how to use bugzilla and I'm not in the mood to write a tut.
I haven't found bug, but I want improvement and the feature?
Code section is open - post your patch there if you done the job.
Or use saloon as usual.
Current post only is about SG 1.1 ioQuake3 backporting review.
There are a lot of things that supposed to be for ioQuake but they alternated in code for SG. To be removed?
Check them closely, compare the ioQuake method and SG one. Maybe we need to create a new one basing on both.
If ioQuake code just needs to be removed it is not a problem - it will be anyway.
Example:
#ifndef SMOKINGUNS
if ( cg.snap->ps.persistant[PERS_TEAM] == TEAM_SPECTATOR) {
return;
}

if ( cg.renderingThirdPerson ) {
return;
}

// set color based on health
if ( cg_crosshairHealth.integer ) {
vec4_t hcolor;

CG_ColorForHealth( hcolor );
trap_R_SetColor( hcolor );
} else {
trap_R_SetColor( NULL );
}

#else
//if a pickable item is in front of the player
if(cgs.gametype == GT_DUEL && cg.introend - DU_INTRO_DRAW <= cg.time
&& cg.introend + DU_CROSSHAIR_FADE >= cg.time){
vec4_t colorFade = {1.0f, 1.0f, 1.0f, 0.0f};

if(cg.introend + DU_CROSSHAIR_START <= cg.time){
colorFade[3] = (float)(cg.time - cg.introend - DU_CROSSHAIR_START)/DU_CROSSHAIR_FADE;
}

trap_R_SetColor( colorFade);
} else if( changeCrosshair ) {
if( isPlayer ) {
// change crosshair to indicate that target is a teammate
drawFriendWarning = qtrue;
}
else {
trap_R_SetColor( colorActivate );
}
}

#endif

As you can see in ioQuake3 code (orange text) is present cg_crosshairhealth which is really nifty feature to be used in SG. So better to include it (this one is done in crosshair patch, I quoted it here as example).
It is not very easy!
It is very tough job actually. Glad you see it.
That why I want everyone who has wish to do it.
I checked all the code, but I haven't found any bug!
Well, happens. Thanks for help anyway and try again later, there are always something to do.
What do you do Bro?
The same. The patch with bugfixes/improvements.

So, that it.
Thank you for attention!
Hope you'll help with this.
Even a little bug to find is a good job. And you'll be proud yourself for sure, believe me :)

Have a good day!
Last edited by Lucky Bro on Tue Sep 15, 2009 10:37 pm, edited 1 time in total.
"You should know that the lies won't hide your flaws/No sense in hiding all of yours/You gave up on your dreams along the way" (c) "Fake it" by Seether
P.S. English isn't my native language.
User avatar
Lucky Bro
Gunslinger
 
Posts: 143
Joined: Mon Mar 09, 2009 4:12 pm



Postby /dev/random » Wed Aug 19, 2009 5:21 pm

Being offtopic; what is that #ifdef SG good for anyways? I think SG is meant to be standalone? Keeping the original code and sg's code in defines, doubles the size of code and is unnecessary bloat.

I'd be interested whether and automated approach would work. Just diff Quake 3 and 1.0 SG, then try to apply that patch to ioQuake aka 1.1 SG. If patching fails, there must be something wrong. This won't catch all errors, but might reduce the amount of time needed for conversion.

If you are really willing to review the whole SG vm code (game, cgame, ui) [or do you talk about the engine as well?], you might also add some doxygen comments ;)
User avatar
/dev/random
Smokin' Amigo!
 
Posts: 410
Joined: Thu Jan 22, 2009 1:58 pm



Postby Lucky Bro » Wed Aug 19, 2009 5:52 pm

/dev/random/,
#ifdef SG is Legacy and sometimes it is differ SG code from ioQuake3 :)
To be removed for sure.

Use any method you want :)

Yeah. I want the review of the whole SG source. Every single file you may found under SVN. Even if bug in readme file.
I'm not sure I want to documentate whole code. Instead I prefer easy-to-read code (it is like shaolin-style :)). And documentation which contains only principles of how the whole model works.
But anyway we have what we have. Any code comments also welcomed.

Hope you'll check the code too :)
"You should know that the lies won't hide your flaws/No sense in hiding all of yours/You gave up on your dreams along the way" (c) "Fake it" by Seether
P.S. English isn't my native language.
User avatar
Lucky Bro
Gunslinger
 
Posts: 143
Joined: Mon Mar 09, 2009 4:12 pm



Postby Tequila » Mon Aug 24, 2009 10:39 pm

:D
Hi bandidos,

Thank you Lucky Bro, I appreciate your wish of "Big Source code Review" and I hope you'll be heard ;)

For /dev/random, the "#ifndef SMOKINGUNS" includes are there until the merge into trunk. It was easier to see the important differences inline for the merge and after 1.1 release this will be cleaner to not have ioQ3 legacy code inline.

Also I will be very busy for 2 more months and I hope to commit an important patch soon but it still need critical tests. But any code review will be welcome and I promess to keep an eye on them ;)
User avatar
Tequila
SG Team
 
Posts: 1100
Joined: Thu Nov 15, 2007 11:33 pm
Location: Montpellier, France



Postby Lucky Bro » Tue Aug 25, 2009 8:27 am

Good day Tequila,

Thx for clean explain about defines :)

Can you do the tag for current revision? Something like "1.1_Beta".
Because if someone will decide to do the code review, it is better to us to know where to look to fix the trouble.
Also it is not required too (tagging), because we may just use revision number which is 264 now.

If the patch is critical better to commit it or post somewhere :) Maybe we'll just fix the troubles earlier :) And it will reduce count of bug reports we may get for sure (not that I think there will be at least one report.. I just do what I think I have to :)).

Have a good months! See you later then :)
"You should know that the lies won't hide your flaws/No sense in hiding all of yours/You gave up on your dreams along the way" (c) "Fake it" by Seether
P.S. English isn't my native language.
User avatar
Lucky Bro
Gunslinger
 
Posts: 143
Joined: Mon Mar 09, 2009 4:12 pm



Closed

Postby Lucky Bro » Tue Sep 15, 2009 10:37 pm

Thanks to everyone who participated. At least tried to think about it. Now you may say that upcoming version is something in what you had took part.

Topic is closed.

Have a nice day.
"You should know that the lies won't hide your flaws/No sense in hiding all of yours/You gave up on your dreams along the way" (c) "Fake it" by Seether
P.S. English isn't my native language.
User avatar
Lucky Bro
Gunslinger
 
Posts: 143
Joined: Mon Mar 09, 2009 4:12 pm




Return to Saloon

Show Sidebar
Show Sidebar

User Control Panel

cron