Closed S-S-X closed 3 years ago
S-S-X
commented
3 years ago Ofc managed to push syntax error and luacheck failure but this would be inaccurate energy storage.
Range guards can be added if needed but I think it is not needed (should try what happens if stack:set_wear(x) is called with invalid values.
If it wont crash then I would not add guards but require tools to provide correct numbers.
S-S-X
commented
3 years ago Well, seems I got values backwards but basic idea works. Just little issue with using tool charges it and charging in bb discharges 🤣 Requires few adjustments to calculations but other than that seems fine.
S-S-X
commented
3 years ago Oh... actually wear value 0 is no wear at all and 1 is max condition, so that's where problem is actually (in addition to having it backwards). So have to go from 1 to 65535 + 1 for overflow instead of 0-65535.
Somehow I was thinking that 65535 is pristine and towards zero is "broken" tool. And also that's why tests are passing even while it works backwards: it is just visuals that are backwards...
S-S-X
commented
3 years ago Maybe I'll still work on this a bit and try to make sure that technic.use_RE_charge will always add at least single point to wear. Other than that only "problem" I can see is with extremely high max charge combined with extremely low charge per but have not yet seen tools where this matters.
If there's tool that has over 65535 individual uses from full charge then that one would not work well (it will be limited to 65535 uses).
edit.
technic.get_RE_charge/technic.set_RE_charge users working with tools that have higher than 65535 uses will need to handle big numbers themselves. Only technic.use_RE_charge would be simple "always-guaranteed" charge use method.
S-S-X
commented
3 years ago Tool charging and discharging rate is high enough to not be likely source of problems. Would be still good to check that input / output error is small enough to not be significant (so that abusing it wont be useful) and maybe even make it always lose charge instead of gaining (so that abusing it would just simulate battery stress and losses).
S-S-X
commented
3 years ago I think I'll merge this and continue testing on main branch.
Upsides:
Downsides:
edit. Actually I found one tool going over this limit, from technic_addons:
Chainsaw MK3: Holds 1,000,000 EU and uses 10 EU/node
OgelGames
commented
3 years ago Yeah, I think the upsides far outweigh that one downside 👍
try to make sure that
technic.use_RE_chargewill always add at least single point to wear
That can be done simpler; round when getting the charge, but floor when setting it: (tested with high-charge tool)
function technic.set_RE_charge(stack, charge)
if charge < 1 then
stack:set_wear(0)
else
local max_charge = stack:get_definition().max_charge
stack:set_wear(65536 - math.floor(charge * 65535 / max_charge))
end
end
function technic.get_RE_charge(stack)
local wear = stack:get_wear()
if wear < 1 then
return 0
else
local max_charge = stack:get_definition().max_charge
return math.floor((65536 - wear) * max_charge / 65535 + 0.5)
end
end
function technic.use_RE_charge(stack, amount)
if technic.creative_mode or amount < 1 then
return true
end
local charge = technic.get_RE_charge(stack)
if charge < amount then
return false
end
technic.set_RE_charge(stack, charge - amount)
return true
endtry to make sure that
technic.use_RE_chargewill always add at least single point to wearThat can be done simpler; round when getting the charge, but floor when setting it: (tested with high-charge tool)
function technic.set_RE_charge(stack, charge) if charge < 1 then stack:set_wear(0) else local max_charge = stack:get_definition().max_charge stack:set_wear(65536 - math.floor(charge * 65535 / max_charge)) end end function technic.get_RE_charge(stack) local wear = stack:get_wear() if wear < 1 then return 0 else local max_charge = stack:get_definition().max_charge return math.floor(((65536 - wear) * max_charge / 65535) + 0.5) end end function technic.use_RE_charge(stack, amount) if technic.creative_mode or amount < 1 then -- Do not check charge in creative mode or when trying to use zero amount return true end local charge = technic.get_RE_charge(stack) if charge < amount then -- Not enough energy available return false end technic.set_RE_charge(stack, charge - amount) -- Charge used successfully return true end
This seems to make it way less accurate than using custom floating point math:
Charging 6 rounds, 10kEU / cycle. Result: error is way higher.
Failure → spec/tools_spec.lua @ 201
Technic power tool charging and discharging extreme ratios t1_65536 can be charged
spec/tools_spec.lua:207: Expected objects to be equal.
Passed in: (number) 59997 Expected: (number) 60000Charging 7 rounds, 10kEU / cycle. Result: error is way higher.
Failure → spec/tools_spec.lua @ 223
Technic power tool charging and discharging extreme ratios t100_6553600 can be charged
spec/tools_spec.lua:231: Expected (number) 69998 to be less than (number) 69501Using extreme ratio tools, 7 rounds, 100 per use, initial charge 700, expected after 6 round: 100, expected after 7 rounds 0. Result: stack destroyed.
Failure → spec/tools_spec.lua @ 235
Technic power tool charging and discharging extreme ratios t100_6553600 can be used
mineunit/itemstack.lua:29: ItemStack:set_wear invalid wear value: 65536These tools registered for tests have extreme max_charge to use ratios (and use values that are likely to cause errors with rounding) like mymod:t100_6553600 has max_charge = 6553600 and calls technic.use_RE_charge(stack, 100).
And mymod:t1_65536 has max_charge = 65536 and calls technic.use_RE_charge(stack, 1).
S-S-X
commented
3 years ago That Chainsaw MK3 from technic_addons actually also somewhat works when cutting large enough area, anything more than just few nodes.
21 x 11 x 21 area actual charge used 48508 where accurate use would be 48510 (10 per node).
Took some time... Mineunit is pretty slow cutting trees with chainsaw 🥼
OgelGames
commented
3 years ago Ah, I see the problem with my code, it floors the value when charging, but it should only floor it when using it, and round when charging.
S-S-X
commented
3 years ago Also stack:set_wear(65536 - math.floor(charge * 65535 / max_charge)) sets invalid value for charge = 1.
edit. Well, that's exactly what you were talking about 🤦
But I've not found problems with current code so unless you got something some a lot simpler solution then I guess current is what should be used.
S-S-X
commented
3 years ago Well, cleaned up negative wear value hack (while kind of nice not sure if it is really safe with engine updates...). Also made it calculate conversion factor during registration and use it instead of max_charge.
S-S-X
commented
3 years ago Maybe max_charge should also be technic_max_charge instead...
OgelGames
commented
3 years ago But I've not found problems with current code so unless you got something some a lot simpler solution then I guess current is what should be used.
There is one problem, set_RE_charge doesn't do the wear check. I think it should be moved out of use_RE_charge and into set_RE_charge.
This code should work for that: (idk how to run tests locally)
function technic.set_RE_charge(stack, charge)
local wear = 65536 - charge * stack:get_definition().technic_wear_factor
stack:set_wear(wear > stack:get_wear() and math.ceil(wear) or math.floor(wear + 0.5))
end
github-actions[bot]
commented
3 years ago S-S-X
commented
3 years ago But I've not found problems with current code so unless you got something some a lot simpler solution then I guess current is what should be used.
There is one problem,
set_RE_chargedoesn't do the wear check. I think it should be moved out ofuse_RE_chargeand intoset_RE_charge.This code should work for that: (idk how to run tests locally)
function technic.set_RE_charge(stack, charge) local wear = 65536 - charge * stack:get_definition().technic_wear_factor stack:set_wear(wear > stack:get_wear() and math.ceil(wear) or math.floor(wear + 0.5)) end
What do you mean by "wear check"? Always rounding and possibly getting "over charged"?
t100_6553600 gets charged too much. not charged enough, error increased (smaller error is already causing loss).
t1_65536 charge is not accurate anymore (very small error, just 1EU).
t100_6553600 lost energy worth 7 charges after used 6 times.
chainsawmk3 accuracy is worse, deviating way more from exact expected use (this might be caused just because of value is bad for testing, I did not really check chainsawmk3 tests but just throwed in copying multimeter test and adding some trees).
I should add few more tests that especially try to abuse charging to gain more than what was pushed from BB as current set does not do that as much.
S-S-X
commented
3 years ago For "how to run tests locally", I'm pretty sure Mineunit is not compatible with Windows so either Windows 10 WSL should be used or some virtual machine running some Linux distro.
From command line you'd just install Mineunit from Luarocks: luarocks install --server=https://luarocks.org/dev mineunit and run it in mod source directory with: cd mods/technic/technic and mineunit
S-S-X
commented
3 years ago I should add few more tests that especially try to abuse charging to gain more than what was pushed from BB as current set does not do that as much.
Yeah, just what I thought. Error in these cases generates small amounts of free energy, however that error is way smaller which indicates that presented idea for technic.set_RE_charge might be correct but calculations done with inaccurate values rounded too early and used in calculations after rounding.
No idea what exactly is causing this, I kind of accepted that error as insignificant but not sure how significant it actually could be. Did not check at all. ...has to be just actual numbers used in tests, and only thing I did not check to other direction.
OgelGames
commented
3 years ago What do you mean by "wear check"? Always rounding and possibly getting "over charged"?
Will all these errors I'm wondering if maybe this isn't such a good idea after all... metadata values seem so much better now...
From command line you'd just install Mineunit from Luarocks:
luarocks install --server=https://luarocks.org/dev mineunit
Hmm, I think I've got Luarocks, I'll have a look into that.
S-S-X
commented
3 years ago Well, of course error is larger because it tries to always go into one direction. I think that's probably not significant because error is similar for both charging and discharging. Probably real reason to do it would be "battery condition simulation" I mentioned which would be bit more complicated anyway and would like to have required maintenance for tools.
S-S-X
commented
3 years ago What do you mean by "wear check"? Always rounding and possibly getting "over charged"?
This:
Will all these errors I'm wondering if maybe this isn't such a good idea after all... metadata values seem so much better now...
I don't see any significant errors there, could you give some example data to make it easier to understand?
OgelGames
commented
3 years ago I don't see any significant errors there, could you give some example data to make it easier to understand?
I mean the errors/inaccuracies in charge values, not errors in the code.
S-S-X
commented
3 years ago So far I've only seen errors increase after testing code suggested in comments while with original code I've only seen errors (which are smaller) balancing out between charge / discharge cycles.
S-S-X
commented
3 years ago I think it would be possible to increase accuracy further with error correction by interpolation based on before/after error difference. Basically if there's error then do linear interpolation for errors wear before/wear after and remove that from value calculated with inaccurate scaled values.
Does this sound about right? This is based on assumption that last change was same as current (which is true for charging and discharging for all but last cycle). Also it is of course balancing instead of direct correction because there simply is not enough data for direct correction.
Also I have not really tested current code in game yet so not sure if it is even needed but probably good to have note here at least.
BuckarooBanzay
commented
3 years ago I merged this PR into the test-server and did some tests with various power-tools (using, discharging, recharging) everything works as expected :+1:
S-S-X
commented
3 years ago Tested charging / discharging tools in loop, for me it seems to be very good. More comments on Discord https://discord.com/channels/513329453741637637/816367895621795861/909717067677175808
It seems like even if there's probably some combinations with exact values it wont be enough to make batterybox + powertool power plants viable, it is simply too slow and it should be easy to if needed because having only wear value leaves all upgrade options open without additional cleanup or leftover data.
It seems that energy losses during charge and recharge are not affecting normal use.
Made another PR for this to make it cleaner and because new PR was not that expensive (and I wanted to have separate branch locally). Tests at least passed unmodified but current tests are not covering that much... also link #233
@OgelGames
Bonus: this should be also option with best possible performance and minimum complexity. API wont change (from what it is at target PR).