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

Wear out tools when player punches object or player. #3631

Closed
wants to merge 2 commits into from
Closed

Wear out tools when player punches object or player. #3631

wants to merge 2 commits into from

Conversation

sofar
Copy link
Contributor

@sofar sofar commented Jan 31, 2016

I'm firmly convinced that swords are meant to be worn out in
core when used by players to hit objects, entities and players.
There are various places here where there is obvious thought of
the wear param, and so this patch isn't too complex to begin
with at all.

getHitParams is the correct place to determine wear. Since
there are no levels for players, we assume a 1 / uses wear
fraction, which then gets scaled to 2^16 just like digging
wear. We propagate the wear to the code from where the punch
happens and apply the wear to the itemstack, and return the
itemstack to the client by updating their inventory.

It's obvious that wear needs to be a u16 and not s16 in the
hitparams, so we correct that.

Entities that hit still need to account for wielded item
wear. I've left a note explaining where this should be handled.

I've tested this code and it correctly wears out and breaks
a sword when hitting a player with a sword. Stone swords
wear out correctly less fast, etc. as well.

@sofar
Copy link
Contributor Author

sofar commented Jan 31, 2016

See also: #3605
Fixes #3615

@nerzhul
Copy link
Member

nerzhul commented Jan 31, 2016

@sofar and if i want to create a magic staff which heal players ? :)

@sofar
Copy link
Contributor Author

sofar commented Jan 31, 2016

@nerzhul if you don't want it to wear down, why choose a tool instead of a craftitem?

In all seriousness though, while looking through this code it's clear that there has been plenty of thought (lua_api.txt even mentions that certain functions should return wear, etc.) to handling item wear, but it's just not ever been coded.

I doubt that this PR is finished in this form, and that some people think that because mods have solved (translate: worked around) this problem that this is a regression, but that's short-sighted and doesn't address the underlying problem.

I'll look at seeing if this needs to hook into optional callbacks so that mods can intercept the punch or intercept the wear.

src/tool.cpp Outdated
if (time_from_last_punch < tp->full_punch_interval) {
float f = time_from_last_punch / tp->full_punch_interval;
//infostream<<"f="<<f<<std::endl;
result_wear /= f;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be result_wear *= f instead?

Copy link
Member

Choose a reason for hiding this comment

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

No, wear is like hunger: the less there is the worse the tool is.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I retract that - it seems the bigger the number the more the wear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is exactly the same as the wear calculation when you dig, I'll need to debug if that calculation is incorrect or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks wrong where I copied it from, indeed.

@sofar
Copy link
Contributor Author

sofar commented Feb 10, 2016

I realized that I shouldn't disregard the item level. Instead I'm including it now in the calculations just as normal tool usage, so diamond tools wear out when punching slower than e.g. wood tools. For simplicity we consider players level 1. This means diamond swords have ~350 uses, and wooden swords ~10.

@C1ffisme
Copy link

Does it check if an object doesn't have the group that the tool does?

I think that hitting a "tree monster" who needs to be hit with an axe, shouldn't damage picks or shovels if no damage is done. Durability is a trade-off. You cause damage, the tool wears down. If you don't cause any damage, realistically you should lose even more durability, but in a game you shouldn't lose any durability, since that really wouldn't be fair to the player.

It would also help the player quickly learn what tools are effective against what enemy.

It also means punching an item with a sword doesn't wear it down.

Sorry if you already coded this. I can't read C++ without taking lots of time and effort into it.

@sofar
Copy link
Contributor Author

sofar commented Feb 11, 2016

@C1ffisme object groups? Never heard of that. If this uses sword tool capabilities I think we can encompass it, if not then we need to assure that mods can intercept and calculate damage and wear for us - I hinted to this in several comments before.

There are several things that already are caught that cause no wear: hitting an inventory or immortal entity, for instance.

Tools should always wear down when used appropriately, even if the effect is diminished. It's not OK to make things "easy" on the player just because you think it's unfair. Unfairness is what makes games interesting and have more depth. If everything is a freebie then there's no challenge. This is why there is falling damage, and why dropped items should disappear, etc.

@C1ffisme
Copy link

@sofar They exist, I coded a fork of PilzAdam's mobs, and made my own from-scratch mob API.

I'm glad that they don't hurt immortal ones. 👍

@sofar
Copy link
Contributor Author

sofar commented Feb 12, 2016

@C1ffisme

I'm glad that they don't hurt immortal ones.

I didn't change damage done, only wear. Why would you expect this patch to change damage ?

@sofar
Copy link
Contributor Author

sofar commented Feb 12, 2016

@C1ffisme

They exist, I coded a fork of PilzAdam's mobs, and made my own from-scratch mob API.

Do you have this code somewhere?

@C1ffisme
Copy link

This is the on_activate() code for animalslite:

on_activate = function(self, staticdata, dtime_s)
    self.object:set_armor_groups(self.armor) -- This line.
    self.object:setacceleration({x=0, y=-10, z=0})
end,

I didn't change damage done, only wear. Why would you expect this patch to change damage ?

Sorry, I worded that wrongly, I meant that immortal mobs do not wear tools. I probably should have though through that sentence more.

@BlockMen
Copy link
Contributor

Wouldn't tools wear out aswell when punching builtin items with your current implementation?

@sofar
Copy link
Contributor Author

sofar commented Feb 17, 2016

yes, any tool would wear if used to punch an entity/player.

@BlockMen
Copy link
Contributor

Must be changed then.

@sofar
Copy link
Contributor Author

sofar commented Feb 18, 2016

So, on first glance, I would be wary of doing that. Tools are essentially craftitems that can wear down, literally. That is what distinguishes them. Restricting wear arbitrarily is a major flaw.

However, there is a concept of armor groups already in core, and there is a concept of tool capabilities. I suppose we can look at the toolcaps and if the tool is essentially the same thing as bare-handed (digging sand with an axe) then we could prevent item wear. Does that seem reasonable?

@sofar
Copy link
Contributor Author

sofar commented Feb 18, 2016

@BlockMen correction, I misread your comment here, or, at least, It needs clarification:

Wouldn't tools wear out aswell when punching builtin items with your current implementation?

I do not know what you consider "builtin items". That term is ambiguous to me. If you mean "Dropped items/itemstacks", then no, this patch does not cause wear to items when you punch them to pick them up.

As I said above in a different reply, immortal items cause no wear when punched - the code returns before any wear is calculated in that case.

This code intents to reduce wear and time when a player exceeds swing
time limits (too fast). The idea is to reduce wear just like damage -
if a player hits something really quickly in succession, subsequent
hits only hit for a fraction.

However, due to the logic, the wear was calculated wrong and hitting
something 5x per second would result in 4 of the hits adding 5x as
much wear as expected.

This doesn't match player expectation - one expects that if you barely
damage something, your sword doesn't wear down a lot more than when
you hit something in a normal fashion.
I'm firmly convinced that swords are meant to be worn out in
core when used by players to hit objects, entities and players.
There are various places here where there is obvious thought of
the wear param, and so this patch isn't too complex to begin
with at all.

getHitParams is the correct place to determine wear. Since
there are no levels for players, we assume players are level 1
objects and scale wear by tool level accordingly, just like
digging. We propagate the wear to the code from where the punch
happens and apply the wear to the itemstack, and return the
itemstack to the client by updating their inventory.

It's obvious that wear needs to be a u16 and not s16 in the
hitparams, so we correct that.

Entities that hit still need to account for wielded item
wear. I've left a note explaining where this should be handled.

I've tested this code and it correctly wears out and breaks
a sword when hitting a player with a sword. Stone swords
wear out correctly less fast, etc. as well.
@sofar
Copy link
Contributor Author

sofar commented Feb 29, 2016

Fix wear calculation when exceeding swing timer.

Copy link
Member

@rubenwardy rubenwardy left a comment

Choose a reason for hiding this comment

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

👍 for implementation, however this needs thorough testing.

As for the concept: Things like swords should be able to damage players without taking any wear, unless they have armour on. So it may be worth adding the ability to disable wear using a group

@raymoo
Copy link
Contributor

raymoo commented Aug 11, 2017

@rubenwardy If you cut someone to the bone though, your sword will be hitting / going through bone, and taking significant wear that way.

@paramat
Copy link
Contributor

paramat commented Aug 11, 2017

Swords wear even if cutting flesh, although it may be slow, and certainly wear used against bone. so i agree with the concept of this.

@rubenwardy
Copy link
Member

Okay, so you could make the sword wear at a normal rate but then wear more if they're wearing armour.

Any other usecases for tools not having wear against entities and objects?

How does this deal with dropped items?

@paramat
Copy link
Contributor

paramat commented Aug 11, 2017

How does this deal with dropped items?

Seems addressed here #3631 (comment)

@rubenwardy rubenwardy added Rebase needed The PR needs to be rebased by its author. One approval ✅ ◻️ labels Nov 28, 2017
@paramat
Copy link
Contributor

paramat commented Dec 6, 2017

@rubenwardy seems the author has tested. I'm fine with always wearing and not being concerned with armour versus flesh etc. Perhaps this can be merged once rebased?
EDIT: Maybe you mean testing to check the rate of wear is reasonable?

@Megaf
Copy link
Contributor

Megaf commented Dec 9, 2017

👍


HitParams(s16 hp_=0, s16 wear_=0):
HitParams(s16 hp_=0, u16 wear_=0):
Copy link
Contributor

@paramat paramat Dec 10, 2017

Choose a reason for hiding this comment

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

Spaces after '='

@@ -143,8 +143,8 @@ DigParams getDigParams(const ItemGroupList &groups,
if(time_from_last_punch < tp->full_punch_interval){
float f = time_from_last_punch / tp->full_punch_interval;
//infostream<<"f="<<f<<std::endl;
result_time /= f;
result_wear /= f;
result_time *= f;
Copy link
Contributor

@paramat paramat Dec 10, 2017

Choose a reason for hiding this comment

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

Seems a significant change, was this a bug?
I can see that wear should be multiplied, not sure about time.

@paramat
Copy link
Contributor

paramat commented Dec 18, 2017

@sofar see line comments.
Can be merged once those are addressed and it is rebased.
If you don't have time to rebase let us know maybe someone else can, but please do explain the change above.

@sofar sofar mentioned this pull request Dec 19, 2017
@sofar
Copy link
Contributor Author

sofar commented Dec 19, 2017

Replaced by #6804, sorry for having to move the thread, github forgot where this PR came from.

@sofar sofar closed this Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants