project-navigation
Personal tools

Author Topic: Bug squashing party  (Read 14363 times)

Offline Bandobras

  • Captain
  • *****
  • Posts: 586
    • View Profile
Bug squashing party
« Reply #15 on: July 18, 2006, 12:18:07 am »
A happy epilogue:

Code: [Select]

<Bandobras> hehe blind coding pays off [22:50]
* Bandobras happy
<Bandobras> why [22:56]
<Bandobras> (baseCurrent->teamInv[i])->i.c[csi.idFloor] =
   NULL;
<Bandobras> does not work?
<TorF> CL_PlaceItem is used to drop an item from a player to the floor ?
<Bandobras> (in cl_campaign.c) [22:57]
<Bandobras> yes and no
<Bandobras> both for taking from the floor
<Bandobras> and for dropping
<TorF> ok
<TorF> is here an "easy" way to reproduce the bug ? [22:58]
<TorF> is there an "easy" way to reproduce the bug ?
<Bandobras> yes and no
<Bandobras> just play a long mission
<Bandobras> and let most of your soldiers die
<Bandobras> and each of the should have at least one weapon
<Bandobras> almost guarantee of "Error: There are more than..." [22:59]
<Bandobras> but the bug seems already squashed
* Bandobras jumps around
<Bandobras> I can make a patch with the fix and with debug statements
<Bandobras> the fix may break other things, though
<Bandobras> and we have to follow Com_DestroyInventory and fix it in other
[23:00]
<Bandobras> places too
<TorF> huhu i hope not :)
<Bandobras> we'll see
<Bandobras> so, upload the patch so you can test?
<Bandobras> just several lines [23:01]
<TorF> i'm not sure i will reproduce the bug
<Bandobras> why
<TorF> especially if the bug is corrected =)
<Bandobras> but similar bugs (I think) are not
<Bandobras> e.g. I think the crash at mission lost [23:02]
<Bandobras> is a similar bug
<Bandobras> perhaps others
<TorF> it's hard to solve problems with so few comments in code and lot of
       global var
<Bandobras> the separation between client and server is a nightmare...
<Bandobras> blind coding on my part...
<Bandobras> and the party really helped (no to mention ssianky) [23:03]
<Bandobras> basically I silenced a warning
<Bandobras> but things may be still broken
* Bandobras shakes [23:04]
<TorF> i hope you have crushed the bug :)
<Bandobras> :)
<TorF> without creating more little vicious bugs :)
<Bandobras> ;)
<Bandobras> why is (baseCurrent->teamInv[i])->i.c[csi.idFloor] = NULL; wrong?
[23:05]
<Bandobras> client/cl_campaign.c:2515: error: invalid type argument of '->'
<TorF> i take a look
<Bandobras>  (in cl_campaign.c) [22:57]
<Bandobras> line 2515
<TorF> because teamInv is declared like this in base_t [23:07]
<TorF> inventory_t teamInv[MAX_WHOLETEAM];
<TorF> replace -> with a . [23:08]
<Bandobras> k
<Bandobras> client/cl_campaign.c:2515: error: 'struct inventory_s' has no
   member named 'i'
<Bandobras> oops
<Bandobras> the problem is I don't know C
* Bandobras embarassed
<TorF> ourg :)
<TorF> it could be a really nightmare for bugs :)
<TorF> look at struc inventory_s declaration : [23:10]
<TorF> typedef struct inventory_s {
<TorF> invList_t *c[MAX_CONTAINERS];
<TorF> } inventory_t;
<TorF> there is only a table of pointers
<Bandobras> so?
<Bandobras> I want to get one such c [23:11]
<TorF> this line could be correct :
<TorF> (baseCurrent->teamInv[i]).c[csi.idFloor] = NULL;
<Bandobras> hmmm
<Bandobras> yes! [23:12]
<Bandobras> compiles!
<Bandobras> What I do now is I grep for all uses of Com_DestroyInventory
<Bandobras> and insert lines like this before them
<Bandobras> client-side uses of Com_DestroyInventory [23:13]
<Bandobras> on the server side things may be even more broken
<Bandobras> but I have no clue
<Bandobras> so
<Bandobras> the reason Com_DestroyInventory is bad
<TorF> it compiles but i'm affraid, have you take a look about invList_t ?
<Bandobras> is that the floor container is shared beteen
<Bandobras> why?
<TorF> set a NULL there can be desastrous if you don't know what you do
<Bandobras> real men don't know what to do, but till code [23:14]
ERC> /poofs
*** poofs: Unknown command
* Bandobras poofs
<Bandobras> but still code
<Bandobras> I mean, I hope invList_t is the same or similar
<Bandobras> to inventory_t, for which this works [23:15]
<TorF> you want to make the container empty ?
<Bandobras> and Com_DestroyInventory has inventory_t argument
<Bandobras> and is applied to invList_t
<Bandobras> so perhaps they are identical?
<Bandobras> or differ only in pointers?
<Bandobras> so the line I add run well with inventory_t [23:16]
<Bandobras> the reason Com_DestroyInventory is bad
<TorF> my idea is : if you want to empty a container, destroy all the items it
       contains first
<Bandobras> is that the floor container is shared between
<Bandobras> soldier and the local entity of floor items
<TorF> ho
<Bandobras> because soldier has floor inventory panel (on client side) [23:17]
<Bandobras> so that he can pick things up
<TorF> ok
<Bandobras> so the floor items would be destroyed twice
<Bandobras> which causes a loop in the free container nodes list [23:18]
<Bandobras> as it was shown earlier today
<TorF> i suppose that the client should be informed that the local container
       is a special container
<Bandobras> the floor container?
<TorF> yes
<Bandobras> I think it can be possible to put a flag
<Bandobras> in containers and set it any time [23:19]
<Bandobras> when the container is shared with the player
<Bandobras> and the ignore it in Com_DestroyInventory
<Bandobras> but it's basically the same work
<Bandobras> you have to take care to flip the flag
<Bandobras> anytime anything happens
<Bandobras> With my approach you have to take care [23:20]
<TorF> yes, to prevent the client to make some bad actions
<Bandobras> to set the floor container to null only when destroying the soldier
<Bandobras> (and when he moves from square to square)
<Bandobras> (which was already done --- the only place done right)
<TorF> the flag could be usefull when soldiers will be allowed to exchange
       some items later [23:21]
<Bandobras>  to set the soldier copy of the floor container to null
<Bandobras> did I say I don't know C
<Bandobras> OK
<Bandobras> I'm not a coder, I'm a fixer
<Bandobras> bugs are my enemies
<Bandobras> that's personal, not intelecual
<Bandobras> but if they exchange things that are on the floor [23:22]
<Bandobras> there is no problem
<Bandobras> the problem is if they exchanged the whole
<Bandobras> floor container local entity
<Bandobras> frankly whole this code is a mess [23:23]
<Bandobras> I hope screemingwithnosound will restructure it properly...
<Bandobras> I guess q2 is a mess
<Bandobras> just that
<TorF> i don't think so
<TorF> problems here are just problems with RULES of our game [23:24]
<Bandobras> hmmm
<TorF> nothing to do with q2
<TorF> restructure such code is really hard work [23:25]
<TorF> bigs risks to create more bugs
<TorF> but if it could be C++, that would be GREAT [23:26]
*** higen (n=ehasting@240.84-48-99.nextgentel.com) has joined channel #UFO:Ai
[23:27]
<Bandobras> sorry, I have C++ [23:28]
<Bandobras> I mean I hate C++
<Bandobras> (don't know it either)
<TorF> ho why ?
<Bandobras> so you mean the fact that floor and inventory portion representing
   floor [23:29]
<Bandobras> are shared is a game-rule?
<Bandobras> (personal)
<Bandobras> the game rule is that an item can be picked up and dropped
<Bandobras> and that things do not dissapear
<Bandobras> from floor
<Bandobras> the rest is GUI [23:30]
<Bandobras> or just implementation of this GUI
<TorF> yes implementation of rules
<Bandobras> frankly, in a functional programming language you'd have
<Bandobras> no problem whatsoever with shared containers
<Bandobras> garbage collection would take care of that [23:31]
<Bandobras> but perhaps the rules, the GUI can be implemented better in C
<Bandobras> how?
<TorF> hum ? [23:32]
<Bandobras> what hum? [23:33]
<TorF> dunno what you mean
<Bandobras> which lines?
<Bandobras> sorry, where I lost you?
<TorF> last lines [23:34]
<TorF> from functionnal programming...
<Bandobras> so, that's a family of languages
<Bandobras> they have garbage collectors, like java, but better
<Bandobras> you don't have to destroy things manually [23:35]
<Bandobras> so no problem with sharing e.g. the floor container
<Bandobras> between soldier and floor local entity
<Bandobras> got it?
<TorF> ho
<Bandobras> was it yet?
<Bandobras> was it "yes"?
<Bandobras> :)
<TorF> java is not a functionnal programming [23:36]
<Bandobras> indeed
<Bandobras> but they swiped the some ideas
<TorF> and C++ have not garbage collector
<Bandobras> yes
<Bandobras> so?
<Bandobras> oh, yes, I hate C++ not only because of that [23:37]
<Bandobras> I even like C
<Bandobras> like a nice antic
<Bandobras> an old car
<TorF> and garbage collector has nothing to do with sharing e.g. floor
       container
<Bandobras> oh yes, it has
<Bandobras> with garbage collector we could remove all the code [23:38]
<Bandobras> that takes care of physically destroying containers
<Bandobras> or reusing them, as in this case
<Bandobras> and this is where the bug was hiding
<Bandobras> while destroying (and reusing list cells) [23:39]
<Bandobras> some pointers were lost, the reuse was repeated for them
<Bandobras> and list became cyclic
<Bandobras> then subsequent reuses looped [23:40]
<TorF> if the solution is to don't know where items are, i don't think it's a
       good one
<Bandobras> with garbage collector you just make a copy of the floor container
<Bandobras> and when the soldier moves or dies just forget his copy [23:41]
<Bandobras> and the items on the forgotten copy can be wherever
<Bandobras> this does not matter
<Bandobras> (you also copied the items)
<TorF> we'll never know :)
<Bandobras> ;>
<Bandobras> so that was my option (not realizable) [23:42]
<Bandobras> what is your option for implementing this?
<Bandobras> I heard flags?
<Bandobras> flags on all containers, set when they are copied?
<Bandobras> that is shared?
<Bandobras> (not copied but shared, indeed) [23:43]
<Bandobras> (we only copy the one pointer pointing at them)
<TorF> just a second
<TorF> i don't have read the corresponding code, so i can't give a precise idea
[23:44]
<TorF> you told me that the soldiers manage floor item like all its items ?
[23:45]
<Bandobras> yes
<Bandobras> you know the game inventory screen, yes? [23:46]
<Bandobras> so the floor is the same as, say backpack
<Bandobras> this is nice...
<TorF> yes [23:47]
<TorF> and client could sometime destroy item of floor ?
<TorF> can you give me the line of code where the floor is assigned to soldier
       pliz [23:48]
<Bandobras> (we are talking about client side, yes?)
<TorF> yes [23:49]
<TorF> hum ok
<TorF> there is a call to CL_InvAdd no ?
<Bandobras> momment
<TorF> to add all the floor items to soldier ?
<Bandobras> CL_PlaceItem [23:50]
<Bandobras> that's the function
<Bandobras> the line
<Bandobras> actor->i.c[csi.idFloor] = le->i.c[csi.idFloor];
<Bandobras> no floor container is created
<TorF> yes it is called from CL_InvAdd or CL_InvDel
<Bandobras> but an existing is used
<Bandobras> yes
<TorF> so it is placed in its backpack ? [23:51]
<Bandobras> no
<Bandobras> each local entity (le) has an inventory
<Bandobras> in paricular, a rifle lying on the floor
<Bandobras> has an inventory with backpack slot, left hand slot and floor slot
<Bandobras> but only the floor slot (the container) is nonempty
<Bandobras> for a soldier, just the opposite [23:52]
<TorF> ho right i got i
<TorF> it
<TorF> in csi_t
<Bandobras> only the floor is empty, unless he stands over something
<Bandobras> so when he stands on something
<Bandobras> his floor container is shared with the floor container of the rifle
<Bandobras> ^^the line I cited [23:53]
<Bandobras> containers are just linked lists of items
<Bandobras> so now both the soldier and the rifle have the first cell of the
   list [23:54]
<TorF> hum and what is the "le" param ?
<Bandobras> the local entity
<Bandobras> the rifle [23:55]
<TorF> this function is called when the soldier get (or put) an item from the
       floor ?
<Bandobras> (as everybody knows, a rifle consists of an inventory, flags,
   coordinates...)
<Bandobras> yes
<Bandobras> perhaps also on other occasions
<Bandobras> perhaps just when he stands, or dies
<Bandobras> or appears (on client side things appear and disappear( [23:56]
<Bandobras> (especially dead soldiers)
<Bandobras> (but maybe even alive --- each turn disappear and reappear anew)
[23:57]
<Bandobras> ( I don't know)
<TorF> i really don't understand how this works [23:58]
<Bandobras> ;) [23:59]
<Bandobras> perhaps, I may be mistaken, on the end of each turn
<TorF> and i don't understand why actor get the le floor [00:00]
<Bandobras> soldiers disappear (CL_Perish)
<TorF> i don't understand what the actor floor become
<Bandobras> and then server sends them anew
<Bandobras> oh, that
<Bandobras> actor is soldier
<Bandobras> his floor was empty
<Bandobras> (soldiers don't carry anythind in their floors, they use
   backpacks, hands) [00:01]
<TorF> are you sure floor was empty ?
<Bandobras> no
<Bandobras> :)
<Bandobras> I hope so
<Bandobras> and I hope there is only one le on the floor, except the soldier
<Bandobras> if not...
<Bandobras> this can be tested
<TorF> the CL_PlaceItem function is called at the begining of each round ? how
       many time ?
<Bandobras> I don't know [00:02]
<Bandobras> perhaps some of the client things are persistent
<Bandobras> (for low net traffic)
<Bandobras> and perhaps all is sucked from the server
<Bandobras> each turn anew, I don't know
<Bandobras> it appears CL_PlaceItem is called many times
<Bandobras> but perhaps these are other function that call Com_DestroyInventory
[00:03]
<Bandobras> (sorry mixed it up, CL_PlaceItem does not call
   Com_DestroyInventory)
<Bandobras> (but if things are destroyed, they have to placed)
<Bandobras> ( with CL_PlaceItem or with CL_Appear) [00:04]
<Bandobras> but CL_PlaceItem is called many times without picking
<Bandobras> things up or dropping
<Bandobras> debug says it [00:05]
<Bandobras> see the log on the forum
<Bandobras> "doubled entity:" is CL_PlaceItem
<Bandobras> and I did not pick things up (perhaps one time, or two) [00:06]
<Bandobras> how's the party?
<Bandobras> we're having bug-squashing party from noon to midnight today
[00:07]
<Bandobras> Woo-hoo
* Bandobras booms
<TorF> yep
<TorF> too hard for me, i need comments in code :) [00:08]
<Bandobras> thanks for your help with (base->teamInv[i]).c[csi.idFloor] = NULL;
[00:09]
<Bandobras> I actually like blind coding
<Bandobras> I don't get attached to the code
<Bandobras> don't feel offended with the quality
<Bandobras> etc.
<TorF> good luck [00:11]
<Bandobras> :) [00:12]
*** fatty (n=rotkaepp@W2390.w.pppool.de) has joined channel #ufo:ai [00:15]
<TorF> hum i got an idea [00:16]
<Bandobras> ?
<TorF> later an actor should not destroy its floor inventory
<Bandobras> so will he drag it with him? [00:17]
<TorF> here void CL_EntPerish( sizebuf_t *sb )
<Bandobras> when he moves, e.g
<Bandobras> ?
<TorF> when le is an actorf
<TorF> actor :) [00:18]
<TorF> just before the Com_DestroyInventory
<TorF> maybe set a NULL to the floor inventory
<Bandobras> if (le->type == ET_ACTOR)
<Bandobras>        le->i.c[csi.idFloor] = NULL;
<Bandobras> I did it:)
<TorF> ho :)
<Bandobras> that's when the warnings about looping lists stopped
<Bandobras> !
* Bandobras dances
<TorF> it doesn't work ?
<Bandobras> works OK
<Bandobras> :) [00:19]
<Bandobras> probably
<Bandobras> apparently
<Bandobras> so, you won
<Bandobras> ex equo
<Bandobras> :)
<Bandobras> that's the line that squashed today's featuring bug
<Bandobras> but there are other places [00:20]
<Bandobras> where Com_DestroyInventory runs
<Bandobras> and perhaps...
<Bandobras> so I try to patch them all and see...
<Bandobras> (because not all buts died)
<Bandobras> (because not all bugs died) :>
<Bandobras> only the one with looping list and a warning [00:21]
<Bandobras> in CL_PlaceItem
<Bandobras> "Error: There are more than..."
* Bandobras claps his hands [00:22]
<Bandobras> hehe double victory
<Bandobras> the bug is doubly squashed
<Bandobras> with two mighty boots
* Bandobras  code-dives again

Offline Bandobras

  • Captain
  • *****
  • Posts: 586
    • View Profile
Bug squashing party
« Reply #16 on: July 18, 2006, 01:34:44 am »
The end. THE BUG is dead, the patch is submitted:

Code: [Select]
A patch that squashes the 1523411 bug. An example of blind coding, but testing shows that probably no new bugs are introduced. Unfortunately grave inventory bugs remain and extrapolation of this simple hack have not succeded in fixing them. Still, may be worth a read (short) even toghether with the IRC logs on the forum:
http://ufo.myexp.de/phpBB2/viewtopic.php?t=237

In the patch there are also some debugging statements an a side-fix or two.

Thanks for the wonderful bug-squashing party. The most important fix is actually independently devised by TorF. Ssianky found the bug. All participants helped in a that way or another.

Offline BTAxis

  • Administrator
  • PHALANX Commander
  • *******
  • Posts: 2607
    • View Profile
Bug squashing party
« Reply #17 on: July 18, 2006, 11:25:50 am »
It sure was a one-man party.

Offline Bandobras

  • Captain
  • *****
  • Posts: 586
    • View Profile
Bug squashing party
« Reply #18 on: July 18, 2006, 11:31:18 am »
Nerd-style. :D

And only males, probably. :?

A classic nerd-party.