Sub HandleSearch

[Vilmen]

Currently that sub looks like this:

>! ```
Sub HandleSearch(ByVal index As Long, ByRef Data() As Byte, ByVal StartAddr As Long, ByVal ExtraVar As Long)
    Dim x As Long
    Dim y As Long
    Dim i As Long
    Dim Buffer As clsBuffer
    Set Buffer = New clsBuffer
    Buffer.WriteBytes Data()
    x = Buffer.ReadLong 'CLng(Parse(1))
    y = Buffer.ReadLong 'CLng(Parse(2))
    Set Buffer = Nothing
>!     ' Prevent subscript out of range
    If x < 0 Or x > Map(GetPlayerMap(index)).MaxX Or y < 0 Or y > Map(GetPlayerMap(index)).MaxY Then
        Exit Sub
    End If
>!     ' Check for a player
    For i = 1 To Player_HighIndex
>!         If IsPlaying(i) Then
            If GetPlayerMap(index) = GetPlayerMap(i) Then
                If GetPlayerX(i) = x Then
                    If GetPlayerY(i) = y Then
                        ' Change target
                        If TempPlayer(index).targetType = TARGET_TYPE_PLAYER And TempPlayer(index).target = i Then
                            TempPlayer(index).target = 0
                            TempPlayer(index).targetType = TARGET_TYPE_NONE
                            ' send target to player
                            SendTarget index
                        Else
                            TempPlayer(index).target = i
                            TempPlayer(index).targetType = TARGET_TYPE_PLAYER
                            ' send target to player
                            SendTarget index
                        End If
                        Exit Sub
                    End If
                End If
            End If
        End If
    Next
>!     ' Check for an npc
    For i = 1 To MAX_MAP_NPCS
        If MapNpc(GetPlayerMap(index)).Npc(i).Num > 0 Then
            If MapNpc(GetPlayerMap(index)).Npc(i).x = x Then
                If MapNpc(GetPlayerMap(index)).Npc(i).y = y Then
                    If TempPlayer(index).target = i And TempPlayer(index).targetType = TARGET_TYPE_NPC Then
                        ' Change target
                        TempPlayer(index).target = 0
                        TempPlayer(index).targetType = TARGET_TYPE_NONE
                        ' send target to player
                        SendTarget index
                    Else
                        ' Change target
                        TempPlayer(index).target = i
                        TempPlayer(index).targetType = TARGET_TYPE_NPC
                        ' send target to player
                        SendTarget index
                        Exit Sub
                    End If
                End If
            End If
        End If
    Next
End Sub
```

Take a look at this part:

>! ```
If TempPlayer(index).target = i And TempPlayer(index).targetType = TARGET_TYPE_NPC Then
                        ' Change target
                        TempPlayer(index).target = 0
                        TempPlayer(index).targetType = TARGET_TYPE_NONE
                        ' send target to player
                        SendTarget index
                    Else
                        ' Change target
                        TempPlayer(index).target = i
                        TempPlayer(index).targetType = TARGET_TYPE_NPC
                        ' send target to player
                        SendTarget index
                        Exit Sub
                    End If
```
Shouldn't that Exit Sub be after the End If?

[iSKweek]

No. It is exiting the sub if a condition isn't met.

[Vilmen]

I'm pretty sure I'm correct. There is no need to loop more if you either removed the old target because u pressed on the same again. Or got a new target. In both cases, the looping should stop.

[iSKweek]

The loop is going through all available NPCs on the map. If it didn't exit out of the sub it would just keep selecting a new target which would be annoying.

[Vilmen]

The current is like this:
```
If TempPlayer(index).target = i And TempPlayer(index).targetType = TARGET_TYPE_NPC Then
                        ' Change target
                        TempPlayer(index).target = 0
                        TempPlayer(index).targetType = TARGET_TYPE_NONE
                        ' send target to player
                        SendTarget index
                    Else
                        ' Change target
                        TempPlayer(index).target = i
                        TempPlayer(index).targetType = TARGET_TYPE_NPC
                        ' send target to player
                        SendTarget index
                        Exit Sub
                    End If
```
I think it should be like this:
```
If TempPlayer(index).target = i And TempPlayer(index).targetType = TARGET_TYPE_NPC Then
                        ' Change target
                        TempPlayer(index).target = 0
                        TempPlayer(index).targetType = TARGET_TYPE_NONE
                        ' send target to player
                        SendTarget index
                    Else
                        ' Change target
                        TempPlayer(index).target = i
                        TempPlayer(index).targetType = TARGET_TYPE_NPC
                        ' send target to player
                        SendTarget index
                    End If
                Exit Sub
```This way, the new target is either "the same target, therefore it sets to 0" or a "new target, set it to = 1". And when 1 of those has rune. The sub should exit.

[iSKweek]

But then it won't find the target.

Edit: I could be wrong with all of this (I haven't fiddled with EO for a while now) but I'm pretty sure it works like that. Might have to wait for Robin or someone more experienced to confirm it.

[RyokuHasu]

WILL ITS MADE THAT WAY FOR A REASON! DON'T TOUCH IT!

[Vilmen]

It doesnt make any sense in my head. Im like 100% sure its wrong.

[iSKweek]

Like I said, wait 'til someone can actually confirm it. Or you could just change it and see what happens.

[RyokuHasu]

FINE THEN, run the damn thing and quit whining about it! XD

[Vilmen]

I changed it the moment I saw it. It's not a big thing really.

_The only bug that can come from keeping it as it was, is that if you have 2 npcs next to each other horizontally. And you have target on the one on the right. And they both step to the right. And at the same time you pressed on the right npc. Then the target could switch over to the other npc. Because the loop didnt stop._

I'm not whining about it… Im just trying to fix a small bug in the engine -_-

[Justn]

Hey William can or anyone else confirm if this is an actual bug fix? I added it but couldn't recreate the scenario you described?

[Displaced]

It's not the loop. It's the position itself. It's set to the next position BEFORE moving.
So you set the new position, and then move the sprits. For you it looks like, you clicked the wrong one, but actually, you don't. The loop is not the problem. It's fine.

[Robin]

Sure. Personally I moved all the targeting client-side so I can add in offset calculations so people can actually click what they see. Having it tile-based with server latency made for a _very_ unhappy experience.

If you're going to try and fix the system then you should really do the same. Nice catch, though.

[Vilmen]

@Justn:

> Hey William can or anyone else confirm if this is an actual bug fix? I added it but couldn't recreate the scenario you described?

It's been confirmed now.

And the scenario I described is hard to create. It would require perfect timing.

[LeaRae]

Yeah, moving the exit is fine.  Doesn't matter a whole lot, you could remove it too but it would select a different target if there were multiples stacked.

I'd restructure the method a bit, move the targetType checks before the For loops and you can eliminate having to loop that many times…