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

Get rid of deprecated metadata #628

Merged
merged 8 commits into from
Jan 22, 2024
Merged

Conversation

cx384
Copy link
Contributor

@cx384 cx384 commented Jan 8, 2024

This solves a part of #601

The deprecated metadata gets converted to a proper ItemStackMetaRef.
All keys stay the same except for:

  • Cans that use can_level now, since they didn't store a serialized table in the metadata before.
  • charge which is now technic:charge, since any item (also from other mods) may have a technic charge which can cause compatibility problems.

(For other keys e.g. mode, the key is special to the item and not universally used, which is not that problematic, I guess.)

There is now only one place left where metadata is used, the sorting function of techinc chests:

local m = st:get_metadata()
local k = string.format("%s %05d %s", n, w, m)

However, I'm unsure how this should be changed.
Getting the whole stack meta as a table and use the amount of keys or a serialized version to sort, may be an option, but I don't think it is justified performance-wise.

To do

This PR is a Ready for Review.

(Maybe change the techinc chests sorting function.)

How to test

  1. Start a game with the old version and get some tools, charge them up, change their modes, fill up some cans.
  2. Switch to the new version and see if the tools still work as expected.

I tested it, and didn't find any problem, but I'm not sure if I missed something.

Compatibility note

Other mods which directly accessed the item metadata of technic items will not work anymore.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Looks good aside from that. Yet not tested.

technic/legacy.lua Outdated Show resolved Hide resolved
technic/legacy.lua Outdated Show resolved Hide resolved
technic/legacy.lua Show resolved Hide resolved
technic/legacy.lua Outdated Show resolved Hide resolved
@SmallJoker SmallJoker self-requested a review January 9, 2024 19:54
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

☑️ Battery box works. Charge gets migrated
☑️ Can level works and gets migrated
☑️ Chain saw works and gets migrated
Flashlight works but seems to be bugged (discharges too quickly), not a problem introduced by this PR.
☑️ Prospector works
☑️ Sonic screwdriver produces a way too loud sound. It's somewhat weird to use but works.
☑️ Vacuum cleaner works

I'll merge these changes in a week unless there are objections. This is a longer delay than what I usually do because it affects existing maps.

@SmallJoker SmallJoker merged commit a08ba2b into minetest-mods:master Jan 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants