search

ForestryMC / MagicBees

Repository for the MagicBees Minecraft mod - a Forestry addon.
Do What The F*ck You Want To Public License
20 stars 18 forks source link

Some bee specialties are produced ten times too often #17

Closed rmunn closed 7 years ago

rmunn commented 7 years ago

If you look back at commit 03068d7ddbac45ee49f7ab067138e5e886c0becf in the code history, you'll see that when MysteriousAges updated the MagicBees code from Forestry 3.4 to 3.6, he updated a lot of the bee-products code to use floating-point numbers instead of integers for the chance of producing a given product. (You might have to click on the "Load Diff" link to see the BeeProductHelper.java diff, since it's a large enough diff that GitHub won't display it by default). Previously, the Forestry API specified production chances as integers that represent a percentage (from 0 to 100), and after the 3.6 chances, production chances were floats between 0.0 and 1.0 instead. However, some of the updated values were incorrect: the integer 8 (representing 8%) in some cases became 0.8 (80%) when it should have been 0.08 (8%), so some specialty products ended up being produced ten times as often as intended. For example, here's the before-and-after part of the diff for the Arcane branch of bees:

// Before:
        ESOTERIC.addProduct(Config.combs.getStackForType(CombType.OCCULT), 18);
        MYSTERIOUS.addProduct(Config.combs.getStackForType(CombType.OCCULT), 20);
        ARCANE.addProduct(Config.combs.getStackForType(CombType.OCCULT), 25)
            .addSpecialty(Config.drops.getStackForType(DropType.ENCHANTED, 1), 9);

// After:
        ESOTERIC.addProduct(Config.combs.getStackForType(CombType.OCCULT), 0.18f);
        MYSTERIOUS.addProduct(Config.combs.getStackForType(CombType.OCCULT), 0.20f);
        ARCANE.addProduct(Config.combs.getStackForType(CombType.OCCULT), 0.25f)
            .addSpecialty(Config.drops.getStackForType(DropType.ENCHANTED, 1), 0.9f);

Notice how Arcane bees now produce Enchanting Drops 90% of the time, instead of 9% of the time as originally intended: that final 0.9 value should have been 0.09.

I've looked back through the Git history, and the incorrect values are still there in the current commits of each of the active branches of the code:

(Note: the first link in each item above just points to the branch, so if code is added to that file in the future, the highlighted line numbers might not be correct any more. The "permalink" in each item goes to the actual commit hash, so its highlighted line number will always be correct).

You can easily figure out all the affected species by looking through the diff for commit 03068d7ddbac45ee49f7ab067138e5e886c0becf, but here's a list of affected species (and products) for your convenience:

rmunn commented 7 years ago

It's actually pretty easy to find the incorrect values, if your text editor / IDE has a regex search feature. Just look for the pattern 0.#, where # is a single digit (i.e., a \d that is not immediately followed by a second \d), i.e. use a "negative lookahead assertion" to say that the \d should not be followed by a second one. Since different regular-expression syntaxes write negative lookahead assertions differently, I can't give an example that will work in every IDE. But it should be pretty straightforward to write such a search expression.

Update: No need for a lookahead assertion, actually, since there's a trailing f after each float thanks to Java syntax. Just search for the regex add(Product|Specialty).* 0\.\df and you'll find all the affected chances. (All the ones where the chance was 10% and it stayed 10% are written as 0.10f rather than 0.1f so it's quite easy to tell which ones were intended to be 10% and which ones were intended to be 1%).

mezz commented 7 years ago

Thanks for the report! Would you mind making a PR? You should be able to edit the values in Github directly and submit a PR with your changes.

rmunn commented 7 years ago

I'm at work right now, but I'll make a PR when I get home.

Note that it'll really be three PRs, since GitHub PRs only target a single branch and I want to fix this in all the active branches.

Elec332 commented 7 years ago

Fixed for the 1.10/1.11 version, the 1.10 branch is not active, so no need to make a pull request for that

rmunn commented 7 years ago

Oh dear. I have a PR almost ready to push, and it conflicts with yours. I'm just now double-checking everything to make sure I got it right; I'll push it real soon, and we can work out the differences once it's pushed.

rmunn commented 7 years ago

Okay, #19 is the one that conflicts. Most of them are trivial conflicts, e.g. where my code has 0.10 and your code has 0.1 (I changed every percentage number to two digits so that nobody would possibly wonder, later, if it was supposed to be 10% or 1%). But some of them are actual conflicts:

That's it, I think. All the rest of the differences I found were trivial.

Elec332 commented 7 years ago

Apatite: 0.10, that sounds like a balanced number to me Carbon: Keep at 0.05, since that was the initial intended value (5%) Endearing speciality: I missed that one, whoops, good catch Botanic: Is already at 0.01 https://github.com/ForestryMC/MagicBees/blob/1.10/1.11-rewrite/src/main/java/magicbees/bees/EnumBeeSpecies.java#L1896

If there are no significant changes (except the wrong value for the Endearing speciality), it might be easier to not merge it, as there are 14 conflicts

Sorry for ruining your PR :(

rmunn commented 7 years ago

No worries. :-) Now that there's been a decision, I'll rebase my commit on top of the current branch and push the rebased commit to the PR branch, and the PR will still be mergeable. (I write C# for my day job and I'm very familiar with Git, so that's not a problem at all for me).

I do think it would be a good idea to merge the PR eventually, since I've made a few more improvements like putting 0.10 / 0.20 / etc everywhere where the values 0.1 / 0.2 / etc were correct. That way there will never again be confusion over whether the 0.1 was supposed to be 10% or 1%. But I'm running out of time for non-work stuff today, so I'll have to wait until tomorrow to rebase this particular PR.

rmunn commented 7 years ago

I've pushed a rebased version of #19 now, and it's mergeable. Its only remaining changes are the apatite, carbon and endearing chances, but I also kept in all the places where I changed 0.1 to 0.10, 0.2 to 0.20, and so on. That will allow anyone who reads the code later on to know that those were the intended values.

19 should be ready to be merged now.