minetest-mods / xdecor

A decoration mod for Minetest meant to be light, simple and well-featured
Other
29 stars 46 forks source link

3d_armor doesn't enchant properly #81

Open aerozoic opened 7 years ago

aerozoic commented 7 years ago

I have a fresh install of 3d_armor and xdecor. I've not made any modifications to either mod. Every single piece of armor of every variety, when enchanted, shows a level of 1.8 on the HUD. I've also done a kill test with diamond armor. Wearing the full set, it took 21 clicks with a steel sword to kill while the enchanted diamond armor only took 10 clicks. :large_orange_diamond:

kilbith commented 7 years ago

@stujones11 Something that could have broke my mod in 3d_armor recently?

ghost commented 7 years ago

cessna151 maybe it makes a difference if you're using clothing, multiskin, and 3d_armor multiskin branch?

aerozoic commented 7 years ago

Yes i am using 3d_armor multiskin branch. If it helps this is the repo for my server: https://github.com/UGXaero/UGXrealms :large_orange_diamond:

mgl512 commented 7 years ago

Something that could have broke my mod in 3d_armor recently?

It's been broken for ages by 3d_armor's new api. We fixed it in our rather heavily updated fork of xdecor but I didn't bother to report. You may want to check this commit: https://github.com/MT-Eurythmia/xdecor/commit/cff1ba48e11bb34af3910d6fbce31c46b3178709. I don't know if it was broken even more after that. Looked ok last month.

I remember that the enchanment table requires armor textures names it doesn't provide and that it doesn't even use them if you provide them. It uses the ones from 3d_armor.

kilbith commented 7 years ago

So now it's clear we can't rely safely on 3d_armor's API from external mods. Thanks for such maintenance, @stujones11 :-1:

Someone needs to import the patches from @mgl512. And please mind to backport fixes in upstream, it's very important.

kilbith commented 6 years ago

Problem radically solved: https://github.com/minetest-mods/xdecor/commit/85b023cfe7d721dae8602bca1d973b7eb8c2cf70

I don't give a crap about unstable APIs causing deal-breaking regressions on other mods.

stujones11 commented 6 years ago

@kilbith Well that should not have been necessary, I am sure there was a much less radical solution had I known about this issue (sorry I missed your 'pings'). I did make substantial re-work to the mod a while back but did my best to maintain backwards compatibility with other mods, obviously I must have missed something though I am sure it would have been a simple enough fix.

Please consider reverting that commit and let me sort this out now that I am aware of the issue.

aerozoic commented 6 years ago

@sofar Would you be willing to reopen this issue and work with @stujones11 to resolve it properly?

Thanks 🔶

stujones11 commented 6 years ago

Unfortunately, it looks like it was broken by adding support for non-fleshy damage groups so there is no easy fix from my end. To be fair though, this has nothing to do with unstable api's as these groups were not previously documented and the method used for enchantment was far from ideal.

There is, however, an open feature request for supporting ItemStackMetaRef which should make things like this much easier https://github.com/stujones11/minetest-3d_armor/issues/114 I will make a PR to re-add 3d_armor support once this is done.

In the meantime, @mgl512 has already provided a (lgtm) patch for this https://github.com/minetest-mods/xdecor/issues/81#issuecomment-325926337 This could have simply been cherry-picked by now, had the original support not been so 'radically' removed.

sofar commented 6 years ago

I'll take a patch that reverts 85b023c and adds the patch from @gml512 on top of it. Please make a PR.

stujones11 commented 6 years ago

@sofar thanks but if I am making a PR it will (hopefully) be a cleaner implementation when I add support for ItemStackMeta. However, I have no objection to anyone else doing this. Since this is now really just a request you can, at least, remove the 'bug' label :)

aerozoic commented 6 years ago

Edit: Here's the simple fix. :large_orange_diamond:

sofar commented 6 years ago

Indentation needs to remain using tabs and not spaces. Please change the spaces to tabs again.