Forgottenserver: Monster:onDropLoot / getLootRandom() not working correctly

[thatmichaelguy]

Before creating an issue, please ensure:

• [✔] This is a bug in the software that resides in this repository, and not a
  support matter (use https://otland.net/forums/support.16/ for support)
• [✔ ] This issue is reproducible without changes to the code in this repository

Steps to reproduce (include any configuration/script required to reproduce)

To make the following steps easy, I suggest changing groups.xml to allow GOD characters to kill monsters and get loot.

1. Pick one monster of your choosing and kill a lot of them.
   -- For my tests I chose Hero and Dragon Lord and killed 250
2. Observe that the monster dropped no rare items, until finally one monster dropped every item in its loot list, including all of its rares.
   -- I tested at both the default 2x loot rate and at 10x

Expected behaviour

Loot distribution of rare items should be spread according to its chance in the its xml

Actual behaviour

Rare items only seem to drop all at the same time.
Seems to only affect loot items with a chance < 700 (approx.)

Environment

Replicated on both Ubuntu 16.04 & Windows 10

[thatmichaelguy]

Removing randomseed from startup.lua & global.lua fixes the "loot explosion" and appears to give a normal loot distribution when tested at very high (50x) loot rate.
Testing at lower loot rate (2x, 3x) wasn't yielding much result over the small sample set (approx. 250).

This causes its own issues, it is not a fix.

[Lyuzera]

It's really working wrong, good point @thatmichaelguy! Removing the randomseeds, worked for me.

[danyelvarejao1]

This is real, there really is this problem, thank you very much @thatmichaelguy.

[Znote]

This should be reproduced/benchmarked with much bigger monster killcount.

I'm sure its possible to create a script that kills 20-40x of same monster every second for a minute to quickly test a large quantity of drop rates.

If you combine something like this:
https://otland.net/threads/autoloot-system-for-tfs-1-x.237607/

with a talkaction that summons lots of param configured monsters in screen and kills them to determine loot accuracy better.

[thatmichaelguy]

I'm happy to run some tests with a much higher kill count to verify a potential _solution_, however I don't see it necessary in verifying this issue.
I must reiterate that removing randomseed from startup.lua & global.lua only fixes the more game-breaking "loot explosion". It causes a new issue whereby all common-loot items will drop on every kill. This is best seen when killing creatures with a high amount of common loot items, like a Demon.
I've done a bit of reading on lua randomseed and there was a fairly robust discussion about it on discord too, but I don't have any solutions to offer at this time.

[iryont]

Seeding each time you want to generate number is just plain stupid and wrong. You cannot possibility expect correct results from the PRNG.

Remove math.randomseed(os.mtime()) from globa.lua in function getLootRandom and add one at the very begnning of global.lua file. That is before dofile('data/lib/lib.lua'). The one within startup.lua should also be removed (useless).

Anyway, I don't know what "mtime" function of os library is, but as far as I remember it isn't standard. That being the case seeding with os.time() will be more than enough.

[Lyuzera]

@iryont os.mtime() is in milliseconds. Witch would be better?

[iryont]

Makes no difference, really. We just need a starting seed at this point. time() is more than enough for this since it's only executed once (when global.lua is loaded by different lua states and script environments).

If you keep reseeding it each random call the PRNG will just reset itself to starting position and therefore distribution won't be valid at all.

Have a look at this simple example:

Of course the seed here will be constant for the whole loop since it's in seconds, but the point still stands. PRNG does reset itself to starting position each randomseed call. That being the case generated values are no longer pseudo random, but purely depend on the current time since the PRNG is seeded with it each time you call a random function to get a random number for the loot.

If we simplify this to hours and minutes instead of milliseconds you would be able to loot all the rares at 00:05 but nothing at e.g. 00:10 since the random values you generate depend on the current time.

[Lyuzera]

@iryont Thanks for explaining! :D

[thatmichaelguy]

Thank you for the explanation. I will apply your suggestion when I get home from work, test it and PR if it works (If nobody beats me to it).

[thatmichaelguy]

Following iryont's suggestion I removed randomseed from startup.lua and from the getLootRandom function of global.lua, and instead used math.randomseed(os.time()) on line 1 of global.lua (before dofile('data/lib/lib.lua')).
The results follow.

Creature: Amazon
Kill count: 504
Loot rate: 3x

| Item name | Expected Drop % | Actual Drop % |
| --- | --- | --- |
| Torch | 1.005% | 3.373% |
| Crystal Necklace | 0.287% | 1.190% |
| Small Ruby | 0.161% | 0.397% |
| Gold Coin | 40% | 93.452% |
| Skull | 80% | 100% |
| Dagger | 80% | 100% |
| Sabre | 23% | 73.61% |
| Brown Bread | 30.333% | 89.286% |
| Girlish Hair Decoration | 10% | 30.357% |
| Protective Charm | 5% | 17.658% |

NOTE: expectedDrop * lootRate = expectedActualDrop (Deviation is to be expected due to sample size)

Based on the above I conclude that this change does indeed fix the lua loot system and will create a PR shortly. I encourage others to test this further.

Side note: Personally I think it is unfortunate that once you apply a loot rate multiplier the chance of a creature with any loot chance > 50% dropping nothing is 0%.
I think it would be 'nice' if we could somehow preserve the chance of a monster dropping nothing while respecting the increase of loot rate for rarer items.

[gpedro]

if you do not use a random seed, all values will be unpredictable.

using randomseed, i can get same results (rate) if two players get drop on same time

[ranisalt]

Remove math.randomseed(os.mtime()) from globa.lua in function getLootRandom and add one at the very begnning of global.lua file. That is before dofile('data/lib/lib.lua'). The one within startup.lua should also be removed (useless).

I would do the opposite, as startup.lua is very clear about it's purpose and global.lua shouldn't contain startup code.

[iryont]

Since I'm not familiar with TFS it purely depends on how many lua states TFS operates in. If it has multiple lua states then each state loads its own math library, thus you need to seed it globally (in a script which is loaded by every lua state, so e.g. global.lua).

If whole scripting interface of TFS operates within one lua state and all environments are executed within it then math.randomsed within startup.lua will do the trick and it won't be necessary to have it within global.lua.

[iryont]

if you do not use a random seed, all values will be unpredictable.
using randomseed, i can get same results (rate) if two players get drop on same time

It appears than you do not understand how a seed for PRNG works. You just seeded it two times within one second with the same value and you expect different values to be generated. Seed should only be applied once (on startup) and never again to avoid any issues.