Debriefing > Bug Reports & Fixes

[BUG] Arsenal step-up

<< < (2/3) > >>

null:

--- Quote from: JohnRDVSMarston on May 21, 2017, 01:43:45 pm ---I installed once again the game and played a little bit of Arsenal. I won the first round and, to savor the victory, I suicided (in-game). Unfortunately my PC is crap, so it took a bit to die, even so that I only suicided in the second round.

But, there, things came a little different: I killed just one person and I leveled-up (it means, instead of two-kill level-up, it was one-kill LU). Unfortunately, I died later, so I don't know if I was gonna get one-kill level-ups forever or just in the first weapon (probably because the game thought that I was using the last weapon, the knife).

I was playing in the "Bonds Only" server... Something on the line, I don't remember. The point is: It was vanilla (Ice-ice,baby) Arsenal, not modified.

--- End quote ---

Hey thanks for reporting, I've spent hours working on this. I'm also glad its going to be in the new patch.

The problem was "KillsPerPlayer" got mixed up with "self.pltracker[player][TR_LEVELKILLS]"  in the calculation for suicide, so instead of removing one kill from total player kills ("self.pltracker[player][TR_LEVELKILLS]"), it removed one Kill from total kills to level ("KillsPerPlayer"), so no matter what the server owner sets its always one suicide and then the next kill to level OR KillsPerPlayer - 1 and then the next kill to level, which is the bug.

The reason this only works on the very first level is because the function "ar_IncrementLevel" does not equal levels greater than zero "(self.pltracker[player][TR_LEVEL] + amt > 0)"    ---> ("self.pltracker[player][TR_LEVEL] == 1" and "amt == -1")  1-1 equals zero, but not greater than zero so the ifelse statement resets the Kills and Levels of the player.  The "ar_IncrementLevel" function is before the kills calculation above so it pretty much does nothing here. If  "self.pltracker[player][TR_LEVEL]" equal 2 then the problem is accounted for in the function.

There is a one line fix, seen here: https://github.com/goldeneye-source/ges-python/pull/6/files

Entropy-Soldier:
Dang, how could I have missed that commit?

Sorry to say this but you're not replicating the intended functionality, which IS to use KillsPerPlayer.  The idea is that you don't lose an entire level and all your kills when you drop a level, you only lose however many kills you just crossed the threshold by.  The comment isn't entirely clear about this so I guess that's my fault but outside of that your change still makes no sense to me.


For some context, this function is only called to:
     reset the player's kills to 0, which doesn't execute this codeblock
     increment/decrement the kill counter by 1

In the second case, only decrementing the kill counter can ever lead to this code block being executed.  The relevant line is, of course:


--- Code: ---
    # Advance the given player's level kills by the given amount.
    def ar_IncrementKills( self, player, amt ):
        self.ar_SetKills( player, self.pltracker[player][TR_LEVELKILLS] + amt )
--- End code ---


Your modified function:


--- Code: ---
    def ar_SetKills( self, player, kills ):
        if not player:
            return
       
        if ( kills >= self.KillsPerLevel ):
            self.ar_IncrementLevel( player, 1 )
            self.pltracker[player][TR_LEVELKILLS] = 0
        elif (kills < 0):
            self.ar_IncrementLevel( player, -1 )
            self.pltracker[player][TR_LEVELKILLS] = max(self.pltracker[player][TR_LEVELKILLS] + kills, 0) # Kills is negative here so we're using it to correct previous level killcount.
        else:
            self.pltracker[player][TR_LEVELKILLS] = kills # No level advancement, just complete the request as asked.
            msg = _( "#GES_GP_GUNGAME_KILLS", str(self.KillsPerLevel - kills) ) # We didn't increment a level which would have caused a level advancement message.
            GEUtil.HudMessage( player, msg, -1, 0.71, GEUtil.Color( 220, 220, 220, 255 ), 1.5, 2 ) # So give a killcount message instead.
--- End code ---


Relevant codeblock:


--- Code: ---
        elif (kills < 0):
            self.ar_IncrementLevel( player, -1 )
            self.pltracker[player][TR_LEVELKILLS] = max(self.pltracker[player][TR_LEVELKILLS] + kills, 0) # Kills is negative here so we're using it to correct previous level killcount.
--- End code ---


but keep in mind that the only function call that can ever trigger this is called with a parameter of "self.pltracker[player][TR_LEVELKILLS] + amt" so we can substitute that in for kills and get:


--- Code: ---
        elif (self.pltracker[player][TR_LEVELKILLS] + amt < 0):
            self.ar_IncrementLevel( player, -1 )
            self.pltracker[player][TR_LEVELKILLS] = max(self.pltracker[player][TR_LEVELKILLS] + self.pltracker[player][TR_LEVELKILLS] + amt, 0) # Kills is negative here so we're using it to correct previous level killcount.
--- End code ---

So the line just became:


--- Code: ---
self.pltracker[player][TR_LEVELKILLS] = max(self.pltracker[player][TR_LEVELKILLS] * 2 + amt, 0)

--- End code ---
Keep in mind that self.pltracker[player][TR_LEVELKILLS] can NEVER be less than 0 so in the current program amt MUST be -1 (possible values are -1, 1) and self.pltracker[player][TR_LEVELKILLS] MUST be 0 (Only 0 - 1 < 0) for this to ever get called.  Thus the simplified version is just:


--- Code: ---
self.pltracker[player][TR_LEVELKILLS] = max(0 * 2  - 1, 0)
--- End code ---


--- Code: ---
self.pltracker[player][TR_LEVELKILLS] = 0
--- End code ---

So, easy fix if you wanted to always reset player kills to 0 on level downgrade.  However this would be horrible on servers that have like...5 kills per level or something so the intended behavior is to just require one more kill until the level can be regained, which is implemented mostly correct but overlooks that there might not always be a level to take off.  The solution is thus rather simple:


--- Code: ---
        elif (kills < 0):
            if self.ar_IncrementLevel( player, -1 ):    # If we can actually take off a level then:
                self.pltracker[player][TR_LEVELKILLS] = max(self.KillsPerLevel + kills, 0) # Kills is negative here so we subtract it from the max kills per level to give the player that many kills to get their level back.
            else
                self.pltracker[player][TR_LEVELKILLS] = 0

--- End code ---

However this also requires ar_IncrementLevel to be adjusted to return true if a level can indeed be taken off, and false otherwise, so it's more than a 1 line fix.  Ultimately I spent more time on this post than I did on the actual fix, so maybe I overlooked something else or there's a better way, but I tested this and it seems to work well enough. 

Honestly I might overhaul arsenal to have a more seamless design for 5.1 to avoid any future confusion. The functions imply and poorly implement more functionality than they actually have, since I adapted previously existing code for a completely new design and then half-assed making it more versatile than needed.  Regardless, The fix I just detailed works in the event I don't get around to fixing up the rest of the code.


I would recommend becoming proficient with algebra if you want to approach problems like this, tools like variable substitution are very handy for contextualizing nested functions.  I've obviously got a lot to learn myself as I haven't been doing this for all that long in the grand scheme of things, but I have to say math comes in handy more often than most other things for me.  I'd also recommend making friends with fellow programmers to pick up more neat tricks, I'm sure there are plenty of other upcoming projects that would love to have your help and full attention.

null:
Thanks E-S, for the fast response, but i think you went through code a little fast, i don't understand your fix, maybe i could see your complete fixed version?

In your solution i saw your if statement calls self.ar_IncrementLevel, but self.ar_IncrementLevel doesn't have a return ('return true' or 'return false') so it will always fail. You can change self.ar_IncrementLevel function as much as you like, but because the kill counter is set after that function is done the kill counter will still be set wrong.   

To explain my line change as simple as possible, if you as a player suicide, you only lose 1 KILL, and will NEVER lose a LEVEL, your kills just bottom out to zero if somehow the kills go negative, because the max() function will always set kills to the highest value which is zero in that scenario. 

Regarding the
--- Code: ---
self.pltracker[player][TR_LEVELKILLS] = max(0 * 2  - 1, 0)
--- End code ---
not sure where this calculation came from, but i didn't write that. The flaw with that is it's the same as writing:
--- Code: ---
(self.pltracker[player]  [TR_LEVELKILLS] = 0)
--- End code ---
  every suicide drops all your kills back down to zero no matter how many kills you have.

Entropy-Soldier:

--- Quote from: Entropy-Soldier on May 29, 2017, 08:40:49 am ---However this also requires ar_IncrementLevel to be adjusted to return true if a level can indeed be taken off, and false otherwise, so it's more than a 1 line fix.

--- End quote ---

So if you want to implement that fix I just posted be sure to do that too.  I only included the relevant code block so I didn't have to bog down the post with another huge function that could be explained away in one sentence.  I'm taking things step-by-step, so stuff like:


--- Code: ---
self.pltracker[player][TR_LEVELKILLS] = max(0 * 2  - 1, 0)
--- End code ---

is to show specific values for inputs in the event that only one specific set of inputs can ever result in that code block being executed, which is the case here.

On that note, it seems like you've made a quintessential misunderstanding of this function.  ar_SetKills is to set the ABSOLUTE kill counter of the target player.  Calling it with a value of 0, for instance, puts them at 0 kills regardless of what value they were at before.  ar_IncrementKills is to increment/decrement the kill counter by the given amount, calling it with 0 will do nothing because it's just adding that value to the kill counter.  Calling ar_SetKills with a negative value does not mean the player suicided, it means we want to take off a kill when there are no more kills to take.  It's in the same vein as some subtraction algorithms, where you decrement the next most significant digit when you're subtracting more than the current one has.

On suicide, ar_IncrementKills is called with a value of -1:

--- Code: ---
    # Advance the given player's level kills by the given amount.
    def ar_IncrementKills( self, player, -1 ):
        self.ar_SetKills( player, self.pltracker[player][TR_LEVELKILLS] - 1 )
--- End code ---

This, as you can see, calls ar_SetKills with a value of "self.pltracker[player][TR_LEVELKILLS] - 1".  So let's replace all instances of the variable "kills" in that function with "self.pltracker[player][TR_LEVELKILLS] - 1".  This is to show what ar_incrementKills is actually doing when called with a value of -1, which it will be in the case of a suicide.



--- Code: ---
    def ar_SetKills( self, player, self.pltracker[player][TR_LEVELKILLS] - 1 ):
        if not player:
            return
       
        if ( self.pltracker[player][TR_LEVELKILLS] - 1 >= self.KillsPerLevel ):
            self.ar_IncrementLevel( player, 1 )
            self.pltracker[player][TR_LEVELKILLS] = 0
        elif (self.pltracker[player][TR_LEVELKILLS] - 1 < 0):
            self.ar_IncrementLevel( player, -1 )
            self.pltracker[player][TR_LEVELKILLS] = max(self.pltracker[player][TR_LEVELKILLS] + self.pltracker[player][TR_LEVELKILLS] - 1, 0) # Kills is negative here so we're using it to correct previous level killcount.
        else:
            self.pltracker[player][TR_LEVELKILLS] = self.pltracker[player][TR_LEVELKILLS] - 1 # No level advancement, just complete the request as asked.
            msg = _( "#GES_GP_GUNGAME_KILLS", str(self.KillsPerLevel - self.pltracker[player][TR_LEVELKILLS] - 1) ) # We didn't increment a level which would have caused a level advancement message.
            GEUtil.HudMessage( player, msg, -1, 0.71, GEUtil.Color( 220, 220, 220, 255 ), 1.5, 2 ) # So give a killcount message instead.
--- End code ---

Note that there are 3 cases to consider here:


--- Code: ---
self.pltracker[player][TR_LEVELKILLS] - 1 >= self.KillsPerLevel
--- End code ---


--- Code: ---
self.pltracker[player][TR_LEVELKILLS] - 1 < 0
--- End code ---


--- Code: ---
else:
--- End code ---

aka

--- Code: ---
self.pltracker[player][TR_LEVELKILLS] - 1 >= 0 && self.pltracker[player][TR_LEVELKILLS] - 1 < self.KillsPerLevel
--- End code ---

With the important consideration that self.pltracker[player][TR_LEVELKILLS] is always >= 0 and < self.KillsPerLevel.

This case, then, can be eliminated right away since you can't subtract from something less that self.KillsPerLevel and end up greater than it. (unless you subtract so much you overflow the integer, which is obviously not the case here.)

--- Code: ---
self.pltracker[player][TR_LEVELKILLS] - 1 >= self.KillsPerLevel
--- End code ---

so our remaining two cases revolve around:

--- Code: ---
self.pltracker[player][TR_LEVELKILLS] - 1 < 0
--- End code ---

if self.pltracker[player][TR_LEVELKILLS] is >= 1, then this will not execute since any non-zero integers will result in a value of 0 or greater.  Thus the codeblock in question ONLY executes when a player kills themselves AND has 0 kills on the current level.  This results in the only values ever being passed to the code after this conditional statement being self.pltracker[player][TR_LEVELKILLS] = 0, and kills = -1.

Then my previous explanation applies.  Just plug in the values:


--- Code: ---
self.pltracker[player][TR_LEVELKILLS] = max(self.pltracker[player][TR_LEVELKILLS]  + kills, 0)
--- End code ---

--- Code: ---
self.pltracker[player][TR_LEVELKILLS] = max(0 - 1, 0)
--- End code ---

--- Code: ---
self.pltracker[player][TR_LEVELKILLS] = 0
--- End code ---

I've actually tested your code just to be sure, and it behaves exactly as I described.  If you die with 0 kills, you lose a level and are downgraded to 0 kills on that level.  Not sure why you think that you can't lose a level since self.ar_IncrementLevel( player, -1 ) gets called regardless of what self.pltracker[player][TR_LEVELKILLS] is set to.  The only instance in which you don't lose a level is when you can't, which is level 1.  Obviously this is the one instance where the original code fails to behave as expected where yours captures the intended behavior, but given that the goal is not accomplished by either setup the third solution is required.


To restate the goal, let's use an example.  This server is running 3 a kills per level setup.  Each kill is represented with a number and each level is represented by set of numbers separated by a |.  The first 3 levels are thus:

012|345|678|....


If we have 4 kills and suicide, we downgrade to 3 kills but stay on the same level.  Thus we just need 1 kill to regain our previous standing which is more or less the standard across all gamemodes.

If we have 3 kills and suicide, however, we should still go down just one kill, to kill 2.  The only difference is that we also go down to the previous level in the process.  The standard is maintained by the fact that just one kill will put you back where you were.  With your fix, suicide after kill 3 will drop you to kill 0, which is not the intended behavior.


Anyway, there's pretty much nothing else to explain here, so if you still disagree with me and can't implement it yourself, I'll provide my entire set of modifications so you can verify that they behave as desired.  I figure that you at least deserved the full explanation as to why your changes won't be included, but anything more than this goes outside my ability to explain.  I guess I could be wrong, in which case no amount of explanation would be sufficient to explain this properly, but given that I've tested both versions of the code and did a pseudo mathematical proof as to why they behave as they do I can't think of a way that's possible.

null:
Thanks E-S for going through all of that and explaining, sorry it took so long to respond. I know because of all the bad bloody lately how teeth clenching it must be to deal with my pain in the arse, i appreciate you going through the extra explanation none the less.

So yep, you're right, i went back over the code again, and my modification was way off, it did break suicide while on zero kills just as you said. I played around with it a bit and i think i got your fix right?  https://github.com/Jnull/ges-python/commit/e110548cae7a0982c8ca032dd9c21297fd593a22  (If that's not right I'll just wait for the patch or whichever)

Thanks for the recommendation on Algebra and other projects and stuff, I'll definitely consider that.

Navigation

[0] Message Index

[#] Next page

[*] Previous page