Closed S-S-X closed 2 years ago
I bet existing tools wont be happy with update. Here's prospector that has both old and new metadata fields:
{
[""] = "return {[\"look_radius\"] = \"3\", [\"target\"] = \"default:desert_cobble\", [\"look_depth\"] = \"21\", [\"charge\"] = 0}",
look_radius = "3",
look_depth = "21",
charge = "163620",
target = "default:desert_cobble"
}
I think this can be solved somehow but not sure about exact implementation.
One possible solution could be to completely forget about charge
and use only max_charge
(static) with tool wear value. Currently charge value is accurate, using wear value makes it less accurate in some cases.
Even that will still require migration for other fields but that's not something you'd like to do every use/charge/discharge cycle but instead during tool registration.
Actually even that (dropping charge
) wont really help here as tools have been setting charge values by themselves, there was only partial API available and therefore data storage cannot be really changed without breaking compatibility.
This is not compatible with official Technic mod and also not compatible with tools provided by other mods because API function similar to added technic.use_RE_charge(itemstack, amount)
was not available.
Tools will keep setting metadata like before and checks for available energy simply wont work. Old style data can be accessed without using deprecated functions.
Question is do we want to go dirty way to keep compatibility or would it be better to really switch to current API and break compatibility?
I would vote for new API and break compatibility, other mods should anyway be updated to get rid of deprecation warnings so why not go forward and create API without dirty maximum compatibility workarounds. Would also fit well with upcoming release.
Related commit (Engine API documentation): https://github.com/minetest/minetest/commit/74b670a7930736fb4f43dcb5c9e0a366bf23cad4
I would vote for new API and break compatibility, other mods should anyway be updated to get rid of deprecation warnings so why not go forward and create API without dirty maximum compatibility workarounds.
Totally agree 👍 Other mods can just check for the new API functions, and use the old way if they don't exist.
We should also consider adding some version identifier to the technic namespace, to make it easier for other mods to distinguish between technic versions, something like technic.plus = true
.
edit. moving contents to description
I would vote for new API and break compatibility, other mods should anyway be updated to get rid of deprecation warnings so why not go forward and create API without dirty maximum compatibility workarounds. Would also fit well with upcoming release.
I agree with this.
We should also consider adding some version identifier to the technic namespace, to make it easier for other mods to distinguish between technic versions, something like
technic.plus = true
.
technic.plus
sounds simple and good, this could also include version number. Might be useful to have technic.version
too but technic.plus
for sure is safer and simpler option if someone else got idea to also add technic.version
.
We should also consider adding some version identifier to the technic namespace, to make it easier for other mods to distinguish between technic versions, something like
technic.plus = true
.
technic.plus
sounds simple and good, this could also include version number. Might be useful to havetechnic.version
too buttechnic.plus
for sure is safer and simpler option if someone else got idea to also addtechnic.version
.
I like the name technic.plus (.version would work for me too). Would prefer a number rather than a boolean there. Something like an international date (20211104 today) would allow easy comparisons in additon to simple checks if existing.
I've been working on rewriting power tool registration to cleanup tools bit more, increase compatibility with old tools a bit and reduce footprint even more for new tools.
Function technic.refill_RE_charge
being simply wrapper is removed from public API, technic.set_RE_charge
serves same purpose and more.
Basic description for upcoming update:
-technic.register_power_tool("technic:flashlight", 30000)
-minetest.register_tool("technic:flashlight", {
+technic.register_power_tool("technic:flashlight", {
description = S("Flashlight"),
inventory_image = "technic_flashlight.png",
- stack_max = 1,
- wear_represents = "technic_RE_charge",
- on_refill = technic.refill_RE_charge,
+ max_charge = 30000,
})
minetest.register_craft({
Increased compatibility with old tools still wont allow skipping tool updates completely but allows minimal changes for tools, basically only this will be needed:
- local meta = minetest.deserialize(itemstack:get_metadata())
+ local meta = technic.plus
+ and { charge = technic.get_RE_charge(itemstack) }
+ or minetest.deserialize(itemstack:get_metadata())
If digtron causes troubles with digtron.tap_batteries
it might well be that it caches function pointer internally, I've noticed digtron doing a lot of caching like this for internal stuff.
If this happens then next options are either update digtron mod (unlikely even while it is already buggy also in this area) or fork it.
Added basic interaction integration tests with technic:multitool, just verifies that tool has certain fields after registration, BB can charge it and charge is used when player uses tool.
(this uses technic:solar_array_hv's instead of technic:hv_solar_array's for battery box charging which probably causes tests to fail when merged with #230 edit. actually tested and seems to be fine...)
Updated tests to include very basic API functions and simple custom tool registration test / custom tool play test.
Should we test compatibility stuff? No? When it breaks it just breaks. I believe it most probably is not good idea in long run but if you have idea why it would be useful then it is not complicated to add.
I did some testing (of tools, didn't test digtron), and everything seems to be working correctly, however I found one significant issue; tools that have been charged before this version don't function, despite looking like they have charge.
I think get_RE_charge
should check and convert the old charge value:
function technic.get_RE_charge(stack)
local meta = stack:get_meta()
local charge = meta:get_int("charge")
if charge == 0 then
local old_meta = minetest.deserialize(meta:get(""))
if old_meta then
charge = old_meta.charge or 0
meta:set_string("", "")
meta:set_int("charge", charge)
end
end
return charge
end
I did some testing (of tools, didn't test digtron), and everything seems to be working correctly, however I found one significant issue; tools that have been charged before this version don't function, despite looking like they have charge.
I think
get_RE_charge
should check and convert the old charge value:function technic.get_RE_charge(stack) local meta = stack:get_meta() local charge = meta:get_int("charge") if charge == 0 then local old_meta = minetest.deserialize(meta:get("")) if old_meta then charge = old_meta.charge or 0 meta:set_string("", "") meta:set_int("charge", charge) end end return charge end
Yeah, that's known issue and I was not yet able to find good solution for it. While mostly harmless it is very confusing and should be fixed somehow. However I'd like to avoid doing that ^^ as much as possible...
Well, I've already tried to look if there's any way to reset old tools with some one shot operation that would work like LBM but so far that seems to be way too complicated. Maybe that's really only viable solution here... at least it could be added to compat
and possibly allow disabling those with configuration.
meta:set_string("", "")
however should not be done because tools might use that metadata for other things (like prospector did and it will also lose configuration, leaves old data which is not nice but maybe still better to let tool decide what to do with old data).
Should we test compatibility stuff?
Well I tested powerbanks
and it worked as intended, got warnings and nothing crashed.
at least it could be added to
compat
and possibly allow disabling those with configuration.
👍
meta:set_string("", "")
however should not be done because tools might use that metadata for other things (like prospector did and it will also lose configuration, leaves old data which is not nice but maybe still better to let tool decide what to do with old data).
The old charge has to be cleared though, otherwise it will be infinite, so maybe this then:
old_meta.charge = nil
if next(old_meta) == nil then
meta:set_string("", "")
else
meta:set_string("", minetest.serialize(old_meta))
end
Also I noticed, use_RE_charge
doesn't use get_RE_charge
, so the tools using that function will still have the same problem.
Should we test compatibility stuff?
That question is more about automated testing. Like this but with old data / old registration that forces it through compatibility stuff https://github.com/mt-mods/technic/blob/efc03a297087c8fe4222fdb59d9c189328a92a73/technic/spec/tools_spec.lua#L83-L97
meta:set_string("", "")
however should not be done because tools might use that metadata for other things (like prospector did and it will also lose configuration, leaves old data which is not nice but maybe still better to let tool decide what to do with old data).The old charge has to be cleared though, otherwise it will be infinite, so maybe this then:
Yeah just charge
removed and metadata pushed back. But still is there any other way...? Well... having version information on every tool would work but not sure... that also does not sound very good solution. It will prevent loop if tool happens to set charge
again.
Should we test compatibility stuff?
That question is more about automated testing.
Ah, then no, that's probably not necessary.
But still is there any other way...?
The only other option is to let the tools handle it, which probably would be okay, as the problem only occurs with tools that have been updated to use the new API.
The only other option is to let the tools handle it, which probably would be okay, as the problem only occurs with tools that have been updated to use the new API.
One thing just came in to my mind, not sure if it is good idea or not but at very beginning of this PR I was looking for ways to drop charge
from storage completely and only use tool wear.
That would solve this issue completely and make it simpler, reason why I decided to not do that was accuracy of charge value but not sure how important that actually is.
https://github.com/mt-mods/technic/pull/233#issuecomment-955770022 (and previous)
That would be nicest way to do it considering compatibility and seamless upgrade, like stated on original comment it would not make new API magically compatible with old tools but (inaccurate in some cases) charge data would survive.
Charge would be scaled (up or down) to 16bit range and this what causes lost accuracy. It should be overall simpler way to handle charge values, would losing some accuracy be acceptable? Thinking about it batteries in real life are far from accurate and consistent energy storage...
do i understand this correctly: this PR is replaced by #239, right? (this could be closed then?)
do i understand this correctly: this PR is replaced by #239, right? (this could be closed then?)
No, that PR is to drop the metadata charge
value, and use the tool wear instead. (it will merge into this PR's branch)
do i understand this correctly: this PR is replaced by #239, right? (this could be closed then?)
No, that PR is to drop the metadata
charge
value, and use the tool wear instead. (it will merge into this PR's branch)
Currently situation here is:
Some tools will lose previous configuration (prospector mostly, if I remember correctly also drills mode is reset).
Actual data is there but not used by tools anymore, old data is not removed ever unless toll is destroyed. Old data is stored in empty meta field meta:get("")
.
let me know if you need help testing this
I've added validation and default return value for technic.get_RE_charge
so it will accept stack that is not technic power tool.
Also similar for technic.set_RE_charge
but instead of silently accepting it will log error about invalid stack.
While technic.get_RE_charge
can be used to easily check if stack is power tool with charge technic.set_RE_charge
on the other hand should never be used on stack that is not registered technic power tool.
Logging error instead of crashing seems to be good way to communicate this and check itself is not expensive or complicated.
I should probably still go through code and see if there's some clear mistakes left around: mostly battery boxes.
This is somewhat interesting PR https://github.com/minetest/minetest/pull/11110 and might allow actually using wear calculated by engine for simple tools in future. Might not be possible as there's also other tool handling involved but there's a chance this would effectively allow removing wear calculations completely from simple tools without need to update API introduced in this PR.
Added pull requests for technic_grass_clean
Added pull request for pocketnuke:
Added PRs for mychisel
Added PRs for antipest:
@BuckarooBanzay can we test this on Pandorabox test server? Technic tool mods should be updated for testing, so far only few have already merged compatibility PRs.
These would require using fork for testing:
Already some time since other PRs merged but not sure about test server state... So these should be ready but must be up to date:
Was there other Technic power tools not included in Technic itself on Pandorabox?
Also added data migration for prospector. Bit better UX as that one actually stores some configuration data.
Was there other Technic power tools not included in Technic itself on Pandorabox?
Only headlamp
I think, though that should work correctly (tested on the previous commit here).
Was there other Technic power tools not included in Technic itself on Pandorabox?
Only
headlamp
I think, though that should work correctly (tested on the previous commit here).
Did some basic testing with headlamp and it worked without problems, should be fine 👍
Cleaned up commits. Previous: a4b00ec Metadata migration for prospector 09d898f Fix tool spec, mod loading was updated fd24d80 Interaction and API tests for power tools a2a4203 Drop charge value completely 0998183 Remove deprecated power tool functions 9026499 Digtron battery holder compatibility c29af5d Use ephemeral sounds for tools 408b6eb Cleanup deprecated metadata access for tools
To: 58373c2 Interaction and API tests for power tools 502bf9e Power tool compatibility shims 5f28b16 Use ephemeral sounds for tools 7206b8d Add power tool API, refactor existing tools
Maybe commit "5f28b16 Use ephemeral sounds for tools" is not needed, only kept it because that one is not exactly about power tool API but more generic stuff that just happens to be here...
Some latest stats for LoC against current master (ec3a52c):
13 files changed, 273 insertions(+), 429 deletions(-)
15 files changed, 393 insertions(+), 430 deletions(-)
2 files changed, 120 insertions(+), 1 deletion(-)
1 file changed, 453 insertions(+)
@BuckarooBanzay can we test this on Pandorabox test server? Technic tool mods should be updated for testing, so far only few have already merged compatibility PRs.
These would require using fork for testing:
* powerbanks (can also ignore powerbanks for now)
Already some time since other PRs merged but not sure about test server state... So these should be ready but must be up to date:
* farming_nextgen * replacer
Was there other Technic power tools not included in Technic itself on Pandorabox?
Also added data migration for prospector. Bit better UX as that one actually stores some configuration data.
Sorry for the delay, test-server is up and running with restore from 2022-03-16 02:00 UTC
:
technic
on branch issue-114-tools
powerbanks
on branch meta-api-fix
No errors on startup, haven't tested anything else yet. I'll update and test a few more open update-PR's when i have some more free-time :wink:
No errors on startup, haven't tested anything else yet. I'll update and test a few more open update-PR's when i have some more free-time 😉
I'll try to test this one later today so no need to worry about it. Will also try to advertise it a bit for some chat discussion if there's other people who'd be interested in some testing 🥼
Everything seemed to work, did not test all tools on pandorabox test server yet as I've been testing also on my own dev server. Mostly tested migration stuff verifying that few selected old tools work and wont be losing charge and are using charge correctly. Power banks worked as items but lost charge immediately when placed to world as nodes, did not test charging because of that as it would require setting charge manually and better to test after node meta things is fixed.
edit. SwissalpS told that "existing in-world bat-packs work fine, only newly placed ones are nerfed :)"
@OgelGames problem with power bank is that it is trying to use Technic power tool API on nodes:
ERROR[Server]: technic.set_RE_charge item not registered as power tool:powerbanks:powerbank_mk1_node
For node metadata it should simply be using its own metadata storage implementation instead of Technic power tool API.
Not really sure if that is exact problem or if it is just mistake using node name when item name should be used. At least it seems that node and item are different things: powerbanks:powerbank_mk1_node and powerbanks:powerbank_mk1
Not really sure if that is exact problem or if it is just mistake using node name when item name should be used. At least it seems that node and item are different things: powerbanks:powerbank_mk1_node and powerbanks:powerbank_mk1
Yep, I think that is the problem, I might have to change how the placing of powerbanks works... 🤔
Will merge this soon to get clean table for future things, waiting for some time but this has been tested since... I don't know anymore.. but probably gone through more testing than any PR before merge so far and with a many testers too.
I'll add some quick fix for power banks through fork + additional pr first if not updated by then. After that will be updating other mods if needed (and perma-forking those if that's what it takes).
Powerbanks compatibility done here: https://github.com/S-S-X/powerbanks/tree/technic-plus and this fork should be used, this time I also did quick testing in game and it seems to work fine.
Did not open PR for powerbanks compatibility. Will merge this PR tomorrow.
I've pushed a commit to https://github.com/OgelGames/powerbanks/pull/8 to fix the bug (sorry for forgetting about that). I tested it with this PR, the current master and the minetest-mods
version, and it seems to work correctly with all of them now.
I'll be merging the powerbanks PR later today, so if there's anything you think I've missed, let me know :)
Cleanup metadata access functions used by Technic power tools. See issue #114
Option 1, requires compatibility shimsVerify that new metadata access is fully compatible with old tools.Old partially charged tools should work after update.Charge level should not change during update.Verify that tools energy usage has not changed.~~Tools continue playing with manual wear and charge storage reads/writes, storage cannot be updated easily in future. Requires dirty workarounds if deprecated functions will be removed (still manual serialization and data storage in meta field with empty key).~~
Option 2, break compatibility (31abc4e)
For this PR option 2 requires at least allowing charge value input/output for
technic.use_RE_charge
. API proposal, tools never touch wear or charge values directly:technic.use_RE_charge(itemstack, amount)
technic.get_RE_charge(itemstack)
technic.set_RE_charge(itemstack, charge)
Testing
Crashes which is expected as I did remove all guards and wanted it to cause error if someone calls function with invalid stack. However checks are simple and crashes are not fun:accepts invalid stacks and logs red error to notify server admin about problems in some mod code.This list is going towards checking actual digtron bugs, methods are copy paste from digtron masterThis tool will lose configurationData migration added for configurationon_refill
, charging/discharging rejected)Notes
262 insertions(+), 419 deletions(-)
excluding compatibility shims, reduces lines of code significantly while providing usable API.Compatibility
technic_addons.zip
Minimal compatibility Emojigit/technic_grass_clean#3
Full compatibility: minetest-mods/mychisel#9
Minimal compatibility https://github.com/berengma/antipest/pull/1
Minimal compatibility berengma/farming_nextgen#19
(linked just one file / repo, some repos contain more)