search

stujones11 / minetest-3d_armor

Visible player armor & wielded items for minetest
Other
56 stars 98 forks source link

Inventory code cleanup. Moved to own "link" mods #95

Closed bell07 closed 7 years ago

bell07 commented 7 years ago

As presage in https://github.com/stujones11/minetest-3d_armor/issues/26 that's my inventory related changes. I did a short test with each mod and it works for me, but please take your tests too.

stujones11 commented 7 years ago

Looks good but I would prefer to keep the sfinv mod support in 3d_armor as it is now part MTG, it can use sfinv.enabled as your previous implementation did. Also it should display the armor levels like the UI & IP formspecs. However, I can take care of that on merge if you like, thanks for the PR.

bell07 commented 7 years ago

During the testing I see if no inventory support is enabled you are protected by armor that you currently weared, but you cannot wear new, replace or remove them. So I get the Idea maybe that could be useful in some subgames, if an "Armor applying stand" will be developed so you can change the armor on the applying stand only, like the skin change using wardrobe mod. So the sfinv integration optional is required for such mods (if anyone like the idea). But it is your choice, you can skip the sfinv-patch in merge, or modify them after the merge by moving the init.lua to 3d_armor/sfinv.lua. Please take care to modify the PR as you like.

stujones11 commented 7 years ago

If no inventory mod is present (including sfinv) it should default to the original armor.formspec containing armor, preview and crafting slots but it looks like that was broken by an earlier commit. As it stands, at least one of the supported inventory mods is required to make it work.

bell07 commented 7 years ago

The "default" inventory should be removed from MTG in the next time: https://github.com/minetest/minetest_game/pull/1573 so you do not need to support them. Since sfinv there is allways a inventory mod present. The question now is do we like to allow 3d_armor to be used without any inventory integration.

stujones11 commented 7 years ago

The question now is do we like to allow 3d_armor to be used without any inventory integration.

That's a good point, it will be up to other sub-game authors to decided their own implementations, so long as it works 'out-the-box' with MTG I am happy. I think in this case armor_formpage can become the default armor.formspec

bell07 commented 7 years ago

Usually the full modpack is installed. In this case the 3d_armor_sfinv is at the place by default. So if 3d_armor + 3d_armor_sfinv + sfinv are active, it works out of the box. If you remove sfinv for a subgame, 3d_armor_sfinv will be skipped in loading by missing dependency. If you remove the 3d_armor_sfinv from modpack, the 3d_armor and sfinv does each own job, but you cannot change the armor in inventory, as I described above.

stujones11 commented 7 years ago

I see what you mean, I guess it would offer even more flexibility that way.

stujones11 commented 7 years ago

I have tested with IP, UI and sfinv, my only slight concern is the red error messages warning of unsatisfied dependencies. I know they are harmless but they may be confusing to new users. Maybe I should make them soft depends but abort when the mod is not present.

bell07 commented 7 years ago

It is the same behaviour as with hazmat_suit if no technic mod is installed. But you are right, if in newbie eyes the conclusion happens "now I need to install technic mod" is may be ok, but "now I need IP and UI on top" is fatal. If you set all them dependencies to optional, please add a minetest.log warning "mod loaded but unused". Please take the same change for hazmat_suit and technic_armor.

stujones11 commented 7 years ago

Merged ea86aa35f555a8bc471c4cb5990cb1b82691b533