Drop chance is ignored

[theraot]

I'm submitting a...

• Bug report.
• Feature request.

Bug report

What is the current behavior?

The drop chance is ignored

What is the expected behavior?

Whatever we get a drop is determined by its chance

Tell us the steps to reproduce the bug, and if possible share a minimal demo of the problem.

Read the code at Rewards.gd line 27, it currently says:

if drop.chance - randf() > drop.chance

So, we subtract from chance a positive number between 0 and 1 (see randf), and we check if that is greater than chance. I repeat, we subtract a positive, and check if we ended up with more.

There are two ways this expression can go: We get false, or we get a type error.

If we create a new project and use it to run the following code:

func _ready():
	check(0);
	check(1);
	check(-1);
	check(10);
	check(-10);
	check(0.5);
	check(-0.5);
	check(INF);
	check(-INF);
	check(NAN);
	check(-NAN);
	
func check(x):
	print(x - 1.0 > x);
	print(x - 0 > x);

The output will be False 22 times.

[NathanLovato]

Note the project's generally outdated. It could use a complete rewrite, pretty much from scratch. Which I did, in part, by creating a new demo here: https://github.com/GDQuest/godot-2d-jrpg-combat

If you want to report and fix bugs that's fine, just note I'm personally focusing on newer, better-written projects.

[theraot]

I picked an old repository because I worry I'll interfere with somebody else work.

---

I'll fix this. Except I went down a rabbit hole. The function randf is documented to be inclusive. I have confirmed experimentally that randf can output exactly 1 and exactly 0 (it took a while),

One would expect chance = 0 to mean never, and chance = 1 to be always. If I write:

if drop.chance < randf():
    continue;

Then when chance is 0 and randf is 0, that is false, and you would get the drop (it is not skipped). Which is wrong because chance of 0 is never.

If I write:

if drop.chance <= randf():
    continue;

Then when chance is 1 and randf is 1, that is true, and you would not get the drop (it is skipped). Which is wrong because chance of 1 is always.

So, I should write something like

if drop.chance < 1 and drop.chance <= randf():
    continue;

or

if drop.chance <= 0 or drop.chance < randf():
    continue;

And my brain wants to simplify that so badly… And it is so error prone! If I confuse < and <=, or and and or, or 1 and 0, it is wrong. I keep double guessing myself. I want a better way to write it, one where it is easier to notice if it is wrong.

With Microsoft (pseudo)random number generator (which I could use in C#), this is more intuitive, because it does not include the upper bound.

[NathanLovato]

Basically, you could get away with randf() <= drop.chance.

For the rest, if an item has a drop chance of 0, it shouldn't be in the list of items an enemy can drop, that's an error from the designer, and you could use an assert to prevent that.

If you want to be extra sure for the "should always drop" case, you can add a condition.

if is_equal_approx(drop.chance, 1.0) or randf() < drop.chance:
    # Add the items

That solves both problems.

[NathanLovato]

I picked an old repository because I worry I'll interfere with somebody else work.

When translating code to C#, you won't. If you have a doubt, open an issue or super WIP PR to say you'll be translating the code. As long as the repository isn't a work-in-progress, you're good to work on any.

[theraot]

This is what I decided on: if drop.chance < 1 and randf() >= drop.chance:, I have the pull request ready (Edit: I mean, for this fix), I was testing.