Move isInArray to Lua

[WibbenZ]

No description provided.

[raymondtfr]

I think isInArray has been in C++ this whole time probably due to performance concerns, so I don't think it would be a good idea moving it to Lua.

Also as we can see in your code, implementing it its pretty straightforward, so isInArray must be in C++ for a reason.

[WibbenZ]

Yeah you might be right @marksamman

[dbjorkholm]

I ran a quick test of 10 million iterations through the table questDoors (defined in global.lua) looking for itemId 3551, and the C++ implementation were roughly one second slower in this scenario. It should be noticeable slower with strings as well.

Code

local itemId = 3551
for i = 1, 10000000 do
    if isInArray(questDoors, itemId) then
        --
    end
end

C++

2.22s
2.39s
2.22s
2.30s
2.23s

avg: 2.272s

Lua

1.24s
1.31s
1.25s
1.25s
1.24s

avg: 1.258s

[ranisalt]

As I said previously, any time you switch between the interpreter and C++ code, it has a performance impact, so that's probably where you're getting the hit. Another possibility is that LuaJIT (if you're using) is noticing the table is not being modified in the inner loop and hoisting the condition outside e.g. JITed code is:

if isInArray(...) then
    for i = ... do
        --
    end
end

That said, performance is not a concern here, as long as complexity maintains and the code is pretty simple, only basic operations and pairs.

Now for a change I'd prefer a contains or has method in the table metatable (if possible, else just table.contains).

[WibbenZ]

We have always used isInArray so I didn't think about using table.contains (which a lot of people use in other apps).
And if we do change it we will have to create a compat function / rename for it.

IMO we should keep isInArray, 99% of the people know about it and have no problems using it.

[raymondtfr]

Just add it to table metatable as ranisalt suggested (table:find(value), table:contains or table:has ) and add it to compat in compat ❓

not related to this PR
@WibbenZ, I'd like to let you know that after #2076 you would need to update (it gives errors) the script spellbook.lua (that I know of) in actions because of the new changes.

[WibbenZ]

Sure ill do that

@raymondtfr What is the error you are getting?

[raymondtfr]

Lua Script Error: [Action Interface]
data/actions/scripts/other/spellbook.lua:onUse
data/actions/scripts/other/spellbook.lua:7: attempt to index local 'spell' (a boolean value)
stack traceback:
        [C]: in function '__index'
        data/actions/scripts/other/spellbook.lua:7: in function <data/actions/scripts/other/spellbook.lua:1>

[WibbenZ]

Are you sure you update your files?
I can't reproduce it, but I had to download from nightlies since my VS is acting up

[raymondtfr]

I have just unpacked clean datakpack files (downloaded now) to a different folder and downloaded nightlies forgottenserver-31672c24d832f75bc2b67d4e9a103b56a2d39c2f-b951-Win32-Release and it happened too.

Btw, it did not happen to a god character, but did to a normal character (player).

[WibbenZ]

Humm, im using forgottenserver-03483aacbddec173517601f5482f64d15946df14-b950-Win32-Release
So #950
Gonna check it again

[WibbenZ]

[raymondtfr]

The heck 😕 . I am using the item spellbook (id 2175) btw, anyway I'm gonna take a nap, it must be me brains lol.

[WibbenZ]

Yeah thats pretty strange, let's see if someone else reports it :P

[Riverlance]

Here is okay, I think:

Tested on latest TFS (pulled it right now and recompiled).

** EDIT: **
Tested with a player as @raymondtfr said.
Here is the error I got (same):

[WibbenZ]

@brunominervino The code we are talking about has already been fixed in a PR from @ranisalt , it was not related to this PR.
This code works just fine.

[brunominervino]

@WibbenZ Sorry, I haven't understood. :) Ignore my comment hehe

[ranisalt]

table.contains = function(tbl, value)

just like string.split below.

[ranisalt]

You don't have to do this check. Assume it's only possible to call as tbl:contains(value) and, if someone passes another object like table.contains(position, value) it's the scripter fault.

[ranisalt]

Should be even faster now without the type check :)

[ranisalt]

Thanks!