bahrmichael / factorio-tycoon

GNU General Public License v3.0
10 stars 7 forks source link

fix productivity research and industry recipe mangling + tag text anti-clutter option #265

Closed winex closed 9 months ago

winex commented 9 months ago

Hi! First of all, Thanks for the cool mod!

I've ran into some issues, so there are some fixes... hoping commit messages are informative enough to review changes

Looking forward to contribute for the Tycoon mod!

/off I'm currently playing with x1000 science cost, building blocks of 1800 SPM (full red belt) and v0.4.0 update few days ago got me way more currency for my ~4k city, so now i get ~330/min of blue packs consuming around 7-9k currency/min. Not that easy to get from Treasury, though - it's almost 4 red belts (4*1800). Waiting for university productivity update here... :) I didn't touch fishery productivity curve: no research yet, not knowing your target numbers (same 1800/min @ 10 level?) for meat.

Don't know the license yet, but i'd accept any license for such small fixes -- signed-off-by: @winex

winex commented 9 months ago

about global.tycoon_primary_industries table - i still can't trace which function does such weird things. it should be only one named add_to_global_primary_industries()

here are couple of dumps (compacted a bit) after restart:

 580.569 Script log(serpent.block(global.tycoon_primary_industries["tycoon-apple-farm"])):1: {
  nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil,
  {
    __self = "userdata"
  },
  nil,
  {
    __self = 0
  },
  [22] = {
    __self = 0
  }
}
 793.007 Script log(serpent.block(global.tycoon_primary_industries["tycoon-wheat-farm"])):1: {
  [17] = {
    __self = "userdata"
  },
  [19] = {
    __self = 0
  }
}

fresh restart and wheat apple is bugged instead: edit: actually wheat is a good simple array below, just first entries wasn't removed yet - i used DeleteEmptyChunks there

1031.104 Script log(serpent.block(global.tycoon_primary_industries["tycoon-apple-farm"])):1: {
  [17] = {
    __self = "userdata"
  },
  [18] = {
    __self = 0
  }
}
1058.983 Script log(serpent.block(global.tycoon_primary_industries["tycoon-wheat-farm"])):1: {
  nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil,
  {
    __self = "userdata"
  }
}

of course, ipairs() skips such tables completely! edit: tables with [17] = entity

winex commented 9 months ago

found it, finally: https://github.com/bahrmichael/factorio-tycoon/blob/5fa1da67ea193f552963600d3bfe9150fde7c591/control_old.lua#L303 that was a sneaky one in v0.4.0: https://github.com/bahrmichael/factorio-tycoon/blob/5fa1da67ea193f552963600d3bfe9150fde7c591/primary-industries.lua#L13

to use ipairs() and shorthands like #array to get it's length we assume it as a simple array, but this change probably makes it a dict (map, hash table, w/e you call it), for which these just silently fails in Lua... :(

...working on it. assuming we keep array type, but still use pairs() (just not to miss entities anymore) and apply some migration to convert it back from v0.4.0 regression? i'd call it

0.4.1-fix-primary-industries-array.lua

"reverting" this line is okay, but won't fix existing saves and i'm currently playing such one

bahrmichael commented 9 months ago

Thank you very much for the pull request! I'll try to look into it today or tomorrow.

bahrmichael commented 9 months ago

Thank you very much @winex! It took me a bit to understand the problem with the pairs/ipairs. Appreciate the guidance so I can become a better lua developer :)

License is GNU GPLv3 per the Factorio Mod page, I forgot to add that here.

bahrmichael commented 9 months ago

Your changes have been published with 0.4.1. Thank you so much!

winex commented 9 months ago

great! GPLv3 is awesome! :fire:

in Factorio, 2 time intervals are shown to player - the items/s in entity description and items/min in PRODUCTION STATS (default: P key). i use the latter, so i call belts as 900,1800,2700/min, which humans can easily compare to just by looking at stats _it'd be better if result_count was named result_per_minute and been post-multiplied by energy_required/60 in recipes..._

p.s.: for balancing to easily maintainable, all the values should be converted using consts similar to x * TICKS_TO_SECONDS, which is everywhere now. :) i just couldn't take energy_required in the recipe code there as it's not a variable, so just put (30/60) for future substitution

bahrmichael commented 9 months ago

Thank you for the updates! I got that curve wrong, and will update the changelog.