The Watermelon schreef: > On 1/23/07, *Per Inge Mathisen* <[EMAIL PROTECTED] > <mailto:[EMAIL PROTECTED]>> wrote: >> The patch sounds awesome! >> >> On 1/22/07, The Watermelon <[EMAIL PROTECTED] >> <mailto:[EMAIL PROTECTED]>> wrote: >>> Changes:(wz28b.patch) >>> 4.Fixed a bug which prevents the droids from removing 'died' targets >>> for auxiliary weapons. >>> 5.Fixed some net message size count problems with vtol I made when >>> adding multiple weapons to vtol >>> 6.Changed sinf/cosf to trig sin/cos lookup access functions trigSin >>> and trigCos for better performance >>> 7.Changed vectorToAngle in move.c a bit to improve its >>> efficiency('+= 360/4' is weird and 'somevalue/180/2' is pointless >>> too imo...) >>> 8.Changed establishTargetHeight in projectile.c to use pIMD rather >>> than displayImd for structures to fix some weird height >>> problems(hopefully) >> Surely these can be split out into separate patches? It is amazing >> how many hours of bughunting you can save with a properly annotated >> change history in the repository. We are not asking for small patches >> just to save time right now, but mostly to save time in the future >> when someone desperately needs to figure out why line 543 of >> src/obscurity.c was changed to do something really odd. That does >> happen every so often. >> >> So please try to separate out as many of the smaller changes as >> possible, and _especially_ bugfixes. > separated 4,5,7,8 from the 500KB patch,6 is not a bugfix and > it involved many files,so I didnt separate it from the big one. I'm assuming all changes to projectile.c only are change 8? Because I see no mention of the removal of extendRad and it being replaced by iRad (I find both of these names rather non-descriptive btw).
So a question, what is extendRad's purpose in the first place, and why did you remove it? Then as for the changes to establishTargetHeight. Changing the initialization of height to =0, and as such making the switch return 0 upon reaching the default case seems logically, although it is hard to read from the code that height is in fact the return value. So instead of breaking from all of the switch's cases it might be better to return from then with height's value, and if it's a constant use that constant rather than assigning it to a variable and returning the variable. (e.g. `default: return 0;'). Plus one very strange thing I've noticed there. In `case OBJ_DROID:' -> `switch (psDroid->droidType)'. It seems that within that switch statement the total target height gets increased by the weapon or utility's height. The way how this is done however is hard to read (the original code comes from your projectile fix 7 I believe btw), since it first adds the highest point then substracts the lowest. Something like this would probably be much easier to understand: > utilityHeight = ymax - ymin; > height += subHeight; or: > height += ymax - ymin; Which would be a little less easier to understand because of the length of the ymax and ymin parts. So using a temporary as above would be my way. This will optimized away by the compiler anyway. This would probably have prevented the error your fixing here from occurring in the first place (since it would have been more obvious that it's there). PS I'm just trying to explain my point of view on this, not trying to attack your person. -- Giel
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Warzone-dev mailing list [email protected] https://mail.gna.org/listinfo/warzone-dev
