stujones11 / minetest-3d_armor

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

API for player armor reduction / increase effects #85

Closed raymoo closed 7 years ago

raymoo commented 7 years ago

You should use or create an API that allows mods to add armor effects to players without conflicting with 3d_armor. It could be used in potions, trinkets, skill progression, etc.

An existing mod that satisfies this is https://github.com/minetest-mods/armor_monoid. If you think that it shouldn't depend on player_monoids and have its interface based around it, I could rework it into something with a standalone API, with an interface that isn't obscured by monoid stuff.

Basically the interface I want is something like this:

3d_armor itself could be a client to this mod, using contribute_armor instead of directly setting the player's armor ratings.

stujones11 commented 7 years ago

This is an interesting idea and I will give it serious consideration, however, I would like to avoid hard dependency on another mod if at all possible as I fear it could cause problems for new users and existing server installations.

My idea to overcome this problem was to provide on_equip and on_unequip callbacks and have the armor itself increase or decrease armor levels, add/remove physics effects or whatever else, as and when armor is changed. Of course this is not without it's own problems like what to do if maximum levels are reached as well as maintaining backwards compatibility with mods using the existing api.

The pointwise calculation you suggest would overcome the max level problem so possibly a combination of these ideas could be workable if the contribute/uncontribute methods were part of the 3d_armor api.

numberZero commented 7 years ago

@stujones11 You can just include these mods in your modpack. If some user have them already, that should not harm, according to the conflict resolution order, as they are separate mods and not a modpack.

Avoiding dependencies may be a bad practice, it makes modder life harder as amount of different incompatible APIs grows.

stujones11 commented 7 years ago

You can just include these mods in your modpack.

That may be one possible solution I have overlooked, I will certainly keep that in mind.

stujones11 commented 7 years ago

@raymoo I assume you are okay with me bundling your mod with this modpack? I realise that the license permits it but I would still like to have your approval. Maybe the repos can be sym-linked?, I've not done that before so am not sure how it effects clone/download, I'll look into it.

raymoo commented 7 years ago

@stujones11 I'd be glad to have my mod bundled. I should eliminate the player_monoids dependency and add a non-monoid interface, right?

Git allows linking to repositories using submodules, but it means git clone on its own won't clone the submodules, you have to do git clone --recursive and even then doing git pull won't automatically update submodules. Technic (I think?) does use submodules. Github's zip download also does not include submodules, so you would have to manually package releases

stujones11 commented 7 years ago

I should eliminate the player_monoids dependency and add a non-monoid interface, right?

Removing the player_monoid dependency would be good but the rest seems fine as-is, although the override of set_player_armor can eventually go as I would use that code to replace the existing function. I imagine it would still be compatible with player_monoids though, right?

Also, thanks for the heads-up on submodules, I had feeling there was a good reason why I don't see them used much in mt mods.

stujones11 commented 7 years ago

On closer inspection of the code, I am wondering whether it would not make more sense for you to include armor_monoid in your standard monoids and I have 3d_armor depend on player_monoids instead.

I have been reading up on submodules and think they may be usable after all, they are (I think) a good compromise between external hard-dependency and code duplication. I have found a script to automatically generate an archive I can then attach to each release.

stujones11 commented 7 years ago

On reflection, I can simply include both player_monoids and armor_monoid as submodules though I am not sure they need to be separate once the compatibility code can be removed, that is not important.

What would be nice is an update_change (or similar) method to avoid the need to delete then re-add changes, if that is possible? If you'd rather not then I can always add a local wrapper function, it's not a problem.

raymoo commented 7 years ago

If you only want to keep one change, you can use the optional string key argument of add_change, which will overwrite any previous change using the same key. For example, armor_monoid.monoid:add_change(player, { fleshy = 0.5 }, "3d_armor:armor").

Currently armor_monoid includes a compatibility hack for 3d_armor (and a soft dependency on it), so I'll need to remove that before inclusion.

EDIT: I've made a branch where the compatibility code has been removed, so I can merge if you think it's ready.

EDIT2: Might be unclear, but specifying a key means you get that key instead of a dynamic numeric key.

stujones11 commented 7 years ago

If you only want to keep one change, you can use the optional string key argument of add_change

What I meant was how you call the del_change method then re-add it in the current compatibility code https://github.com/minetest-mods/armor_monoid/blob/master/init.lua#L180 I admit I have not had time to study your mod so I had planned to simply replace the current set_player_armor with yours for now. Eventually this will be replaced with a more event driven design using equip callbacks.

I've made a branch where the compatibility code has been removed

Would it be possible for you to push your changes to an upstream feature branch so I can test things? I will do likewise.

raymoo commented 7 years ago

I checked player_monoids to be sure, but those del_changes were redundant, it must have been my mistake or from a version where you did need to delete.

I can't push the changes until later when I have access to my computer.

stujones11 commented 7 years ago

I can't push the changes until later when I have access to my computer.

No problem, it may be the weekend before I get much more opportunity myself after tonight (GMT) but hopefully we can get this added and I can make a new release then.

raymoo commented 7 years ago

I've pushed the branch under the name nocompatibilitything.

stujones11 commented 7 years ago

Thank you, I eventually realized that hard dependency was not even necessary at all, I can simply soft depend and check for existence. I have just created a new 'monoidthing' branch that should use your mods when present. I would appreciate if you gave it a look over in case I missed something obvious, I still do not fully understand how your mod(s) work and it's a bit difficult to test alone.

raymoo commented 7 years ago

I don't see anything obviously wrong. I'll try testing when I get home.

stujones11 commented 7 years ago

At least nothing is obviously broken so I can probably just merge this, my only slight concern was that the effects would stack up every time armor changes but I gather your mod takes care of all that. Also, would it resolve this issue for you?

raymoo commented 7 years ago

Yes, player_monoids shouldn't accumulate effects if they are set at the same key every time. Also merging this would resolve this issue for me.

stujones11 commented 7 years ago

Okay then, I will leave it up to you to update armor_monoid, however I will be making a new release sometime over the weekend and since the current version should still work for now, you might be better off waiting until then so you can specify a version for compatibility.

raymoo commented 7 years ago

I went to add a version tag and found that the current master already had one, so I linked to that for the old version. I've already made added a new version tag for the new version and posted it to the forums.