Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hitech weapons #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robertxgray
Copy link

Hitech weapons use the obsolete get_metadata and set_metadata functions to get and update the technic charge. They have been replaced by get_meta:get_int and get_meta:set_int.

@wsor4035
Copy link

key changes from charge to technic:charge is this something both technics have changed?

bweapons_api/api.lua Outdated Show resolved Hide resolved
The hitech weapons are only compatible with the stable version of
Technic Plus. This change adds support for Technic and Technic Plus
Beta.

It also updates the code to use the new metadata functions instead
of the deprecated get_metadata and set_metadata.
@robertxgray
Copy link
Author

I found a bug after further testing. Here is another version.

@wsor4035 wsor4035 requested a review from S-S-X December 29, 2024 23:36
@BuckarooBanzay BuckarooBanzay added the bug Something isn't working label Dec 31, 2024
@S-S-X
Copy link

S-S-X commented Jan 2, 2025

Didn't forget this one but it might have something to do with linked Technic Plus 1.2.x PR. That would be backporting technic.use_RE_charge(itemstack, charge) from Technic Plus Beta and in turn allow using single path for all latest Technic Plus releases.

Give it a few days and we'll see if we'd get somewhere with that.
After all didn't really do it just for this PR but moreover it would be good to do the same for many high-compatibility features, especially those that have been added to original Technic and Technic Plus Beta but not for Technic Plus stable CDB release.

Comment on lines +418 to +443
-- Technic Plus
local meta = itemstack:get_meta()
local old_metadata = minetest.deserialize(meta:get_string(""))
local charge = nil
if old_metadata then
charge = old_metadata.charge
end
-- Technic
if not charge then
charge = meta:get_int("technic:charge")
end

if charge < technic_charge_per_use then
reload = true
else
if not technic.creative_mode then
charge = charge - technic_charge_per_use
technic.set_RE_wear(itemstack, charge, technic_charge)
if old_metadata and old_metadata.charge then
old_metadata.charge = charge
meta:set_string("", minetest.serialize(old_metadata))
else
meta:set_int("technic:charge", charge)
end
end
end
Copy link

@S-S-X S-S-X Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be enough for both migration and usage with original Technic, Technic Plus stable and Technic Plus Beta if we get mt-mods/technic#391 through:

Suggested change
-- Technic Plus
local meta = itemstack:get_meta()
local old_metadata = minetest.deserialize(meta:get_string(""))
local charge = nil
if old_metadata then
charge = old_metadata.charge
end
-- Technic
if not charge then
charge = meta:get_int("technic:charge")
end
if charge < technic_charge_per_use then
reload = true
else
if not technic.creative_mode then
charge = charge - technic_charge_per_use
technic.set_RE_wear(itemstack, charge, technic_charge)
if old_metadata and old_metadata.charge then
old_metadata.charge = charge
meta:set_string("", minetest.serialize(old_metadata))
else
meta:set_int("technic:charge", charge)
end
end
end
-- Technic, old metadata migration
local meta = itemstack:get_meta()
local old_metadata = core.deserialize(meta:get_string(""))
if old_metadata and old_metadata.charge then
technic.set_charge(itemstack, old_metadata.charge)
meta:set_string("", "")
end
-- Technic, check and use charge
local charge = technic.get_charge(itemstack)
if charge < technic_charge_per_use then
reload = true
elseif not technic.creative_mode then
technic.set_charge(itemstack, charge - technic_charge_per_use)
end

That's already +15 -26 LoC

Considering simplicity of changes and compatibility gains on Technic Plus side for me this seems significant improvement.

Copy link

@S-S-X S-S-X Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also any Technic fork that implement technic.get_charge and technic.set_charge no matter how charge is actually stored.

Though if any of the forks with these are still using old meta then migration would do unnecessary metadata reset each time.

Also I'm not sure if full old metadata reset is safe with these items, if something else round here uses old metadata then that something else would be lost too. However I'd argue full cleanup of old metadata would be very good in case it isn't needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants