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

Allow non-tools to use wear to display a bar #4715

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@thePalindrome
Copy link
Contributor

commented Nov 2, 2016

Do note that destroying items due to damage only occurs on tools.

This change is compatible, even if you load a "damaged" craftitem on an older version.

EDIT: Addresses #6448

@est31

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

What are usecases for this?

I'm not really sure why non-tools should have different behaviour from tools here.

@stujones11

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

What are usecases for this?

Armor might be one, since I am currently forced to register armor somewhat incorrectly as tool items just so they can take damage.

@est31

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

Yes, but I still don't get why items should not be destroyed when the bar is empty, and tools should.

@stujones11

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

Ah, I missed that bit. I would certainly want the armor to be destroyed when the bar is empty.

wear += amount;
return true;
}
if(amount > 65535 - wear && getDefinition(itemdef).type == ITEM_TOOL)

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Nov 2, 2016

Member

Space after the two if's, otherwise fine.

@stujones11

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

I suppose you might want to use the bar for something other than wear but you don't want the item to disappear just because the level became zero. I do that with my shooter mod for gun ammo so there are already ways around that problem, though I am not saying they are elegant.

@Rogier-5

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

I suppose you might want to use the bar for something other than wear but you don't want the item to disappear just because the level became zero.

How about lua callback: on_worn_out() whose default action is to make the item disappear. A non-default action could be to disable the effects of the item/tool (e.g. oil lamp empty: lamp remains, but won't burn. And indeed ammo: weapon remains, but can't fire).

@est31

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

@Rogier-5 that idea sounds good, it would enable people to get rid of hacks like this.

E.g. the return value true of on_worn_out would make the item disappear, and false/nil would make it not disappear.

@thePalindrome

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

I heartily agree that an on_worn_out callback would be a much better implementation than simply relying on mods to properly handle item destruction (which minetest could not reasonably determine)

My personal opinion on the matter is that this pull request be denied, such that the first implementation is the correct one, but I shall leave that decision to those more responsible.

@stujones11

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

I am all in favour of a callback, however, I think that tool items should be given the same option for the sake consistency. The problem would be backwards compatibility, on_wear_out would need to be optional returning nil (not present) when you do want it to disappear (existing behaviour).

@Rogier-5

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

Allow non-tools to use wear to display a bar

@thePalindrome you're not explaining why you actually need craftitems to display a bar ? And why you can't use a tool in those cases ? What properties does a tool lack that craftitems-with-bar possess ?

I am currently forced to register armor somewhat incorrectly as tool items just so they can take damage.

@stujones11: it sounds like your main problem is the fact that 'armor is not a tool', i.e. the name 'tool' is not appropriate for armor. If 'tool' were renamed to 'craftitem_with_level`, that would solve your issue ?

BTW: I do think armor is, technically speaking, a tool, albeit one that protects against damage instead of one that does damage.

@Ferk

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2016

Buckets and books could be, technically speaking, considered tools as well, and yet they are craftitems.
It certainly looks like 'tool' is meant to really mean 'craftitem_with_level' and not really "tool".

Also I don't understand why "tool_capabilities" is a tool thing when you actually can use any craftitem you hold for digging or damaging (so long as it doesn't have an on_use), and there are even tools that implement on_use and actually can't dig or damage (like the hoe).

I think the explanation in lua_api.txt doesn't really hold up. It doesn't even mention wear level (apparently the main difference), and can mislead people to think it implies that only tools can dig and damage.

@thePalindrome

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2016

I'd say the difference between a tool and a craftitem_with_bar is that a lot of code that makes sense for tools (damaging when digging for example) doesn't make sense for craftitems_with_bars (in my example, a battery of sorts that could feasibly be empty), if I took a AA battery and somehow dug a hole in my yard, is the battery now containing less energy?

My main reason for this is to avoid any special regards for tools (like a GregTech style mod that makes a lot of tools worthless)

wear += amount;
return true;
}
if(amount > 65535 - wear && getDefinition(itemdef).type == ITEM_TOOL)

This comment has been minimized.

Copy link
@Ferk

Ferk Nov 5, 2016

Contributor

Shouldn't this 65535 be a #define constant somewhere?

This comment has been minimized.

Copy link
@thePalindrome

thePalindrome Nov 5, 2016

Author Contributor

That was from the original code, so I'm not sure the reasoning behind that.

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Nov 5, 2016

Member

The constant would be called U16_MAX. There can be no higher wear than that, so it might be a good idea to replace this now.

This comment has been minimized.

Copy link
@Rogier-5

Rogier-5 Nov 5, 2016

Contributor

IMHO a better name would be WEAR_MAX. However, if the 65535 is replaced with a constant, even if it's U16_MAX instead of WEAR_MAX, then for consitency it should also be replaced everywhere else where 65536 is used for maximum wear. Using U16_MAX or WEAR_MAX in some places, and 65535 in others makes the code messy, and harder to maintain.

Symbolic constants are not recommended for their own sake. They are not a goal, but a means. The goal is to make code more maintainable. If making a constant symbolic decreases understandability of the code, or maintainability, then it should definitely not be done.

@Ferk

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2016

damaging when digging for example

If the "uses" number for the tool_capabilities is zero you don't get any wear when digging (which is already the default since when you don't define any tool_capabilities it's the "hand" tool the one that is used and that one has uses=0).

I assume for an item like a battery or an armor you would not define any tool_capabilities at all (they'll be nil) when you register it as a tool, so the hand defaults will be used and it would not wear out the item when digging with it.

Btw, I just tested in current master and craftitems do support tool_capabilities.
In builtin, register_tool and register_craftitem are just a wrapper to the same minetest.register_item method, which basically just uses different defaults (values here) depending on the type (and btw, tool_capabilities defaults to nil for both).

It might be interesting to test these two axes in your branch:

minetest.register_craftitem("testmod:axe_craftitem", {
    description = "Craftitem Axe",
    inventory_image = "default_tool_woodaxe.png^[colorize:#044:128",
    tool_capabilities = {
        full_punch_interval = 1.0,
        max_drop_level=1,
        groupcaps={
            choppy = {times={[1]=2.10, [2]=0.90, [3]=0.50}, uses=10, maxlevel=2},
        },
        damage_groups = {fleshy=7},
    },
})



minetest.register_tool("testmod:axe_indestructible", {
    description = "Indestructible Axe",
    inventory_image = "default_tool_diamondaxe.png^[colorize:#404:128",
    tool_capabilities = {
        full_punch_interval = 1.0,
        max_drop_level=1,
        groupcaps={
            choppy={times={[1]=2.10, [2]=0.90, [3]=0.50}, uses=0, maxlevel=2},
        },
        damage_groups = {fleshy=7},
    },
})

See if the craftitem axe also wears out when chopping trees.
If it does, then this PR basically makes crafitems and tools the same.

That's not necessarily a bad thing, imho. I think it's still good to have separate registering and types for the sake of organization (creative mode tabs?), even if functionality-wise they are the same.

Maybe the tool repair logic is the only remaining thing that makes a distinction. But if wear is added to craftitems, then that might as well be added too.

Definitely the docs need to be updated, whether we decide to make tools and crafts the same or not.

@stujones11

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2016

Another consideration is that tool items are not stackable, how would wear level be shown on a stack of craft items? I think @Rogier-5 is correct, in my case at least, it doesn't matter to the player whether armor is registered as a tool item or not so long as it does the job. I do still like the idea of a callback to avoid the need for hacks when you don't want an item to be automatically destroyed.

@Rogier-5

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2016

I'd say the difference between a tool and a craftitem_with_bar is that a lot of code that makes sense for tools (damaging when digging for example) doesn't make sense for craftitems_with_bars

@thePalindrome: taking damage after a hit could just be a difference in behavior. Easily implemented using different callbacks. That might be the same for other types of behavior ?

I even think that it might also be sensible to allow nodes to have a bar as well. The wear could be automatically converted to a param2 value, or to a metadata value when the node is placed, and vice versa when dug. Or a callback could take care of that.

Another consideration is that tool items are not stackable, how would wear level be shown on a stack of craft items?

I'd say that at least tools at 0% and at 100% should be easily stackable. Any two identical tools with the same wear could also be stackable. That may seem strange for tools like axes and picks (and it could be disabled for those), but it would make sense for other types of tools, e.g. tools that have just a few possible levels of wear (e.g. 0%, 50%, 100%).

If you hit something using a stack of tools, then the 'rest' of the stack would be moved to another inventory slot. If none is available, the rest of the stack would be dropped...

Regarding the 'bar': Instead of (or in addition to) a bar, it would also be neat to have different images for tools at different levels of wear. At the moment, the only option is to convert the tool to a different tool...

@paramat

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

Author writes:

My personal opinion on the matter is that this pull request be denied,

It seems to need a better implementation. Also no progress in a year.

@paramat paramat closed this Dec 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.