Skip to content

Conversation

@lisacvuk
Copy link
Contributor

@lisacvuk lisacvuk commented Feb 2, 2020

Solves #5686

To do

This PR is a Work in Progress

  • Fix things like rails not properly changing texture in both wield model and inventory
  • Use ItemStack::getTexture in more places instead of manually checking if metadata exists

How to test

minetest.register_chatcommand("test",` {  
   params = "",  
   func = function(name, param)  
              local player = minetest.get_player_by_name(name)  
              local stack = player:get_wielded_item()  
              stack:get_meta():set_string("texture", "default_leaves.png")  
              player:set_wielded_item(stack)  
          end,  
})  

@SmallJoker SmallJoker added @ Script API Feature ✨ PRs that add or enhance a feature WIP labels Feb 2, 2020
@sorcerykid
Copy link
Contributor

Correct me if I'm wrong, but doesn't this have the potential to break mods that already use the "texture" field of itemstack metadata? As more and more specialized fields like this are added, I'm concerned that this will cause conflicts down the road.

Perhaps it should have been decided that specialized fields be denoted using a underscore prefix or some other convention so that there's never any risk of naming collisions when the API is changed.

@ghost
Copy link

ghost commented Feb 2, 2020

Perhaps it should have been decided that specialized fields be denoted using a underscore prefix or some other convention so that there's never any risk of naming collisions when the API is changed.

While I don't expect that many (any?) mods use this particular field, I definitely support adopting a standard naming convention for metadata fields used by the engine.

@lisacvuk
Copy link
Contributor Author

lisacvuk commented Feb 2, 2020

I can make this use "_texture", it shouldn't be difficult to change description to "_description" as well.

@sorcerykid
Copy link
Contributor

That is great news, and thanks for the clarification!

I am very much looking forward to this PR. I can't even imagine all the potential applications. Once this is merged, hopefully it sets a precedent for the long awaited ability to dynamically change textures of nodes using the same methodology. One step at a time, but at least this is a great step in that direction :)

@lisacvuk lisacvuk requested a review from nerzhul February 2, 2020 17:29
@Skamiz
Copy link

Skamiz commented Feb 8, 2020

I believe it is worth noting that the documentation on item definitions encurages modders to use an underscore for custom item/node definition fields.
https://github.com/minetest/minetest/blob/master/doc/lua_api.txt#L6578

        _custom_field = whatever,
        -- Add your own custom fields. By convention, all custom field names
        -- should start with `_` to avoid naming collisions with future engine
        -- usage.
`

@sorcerykid
Copy link
Contributor

sorcerykid commented Feb 8, 2020

Thanks for the heads up. To be clear, those are item definition fields, which are separate and distinct from metadata fields that are serialized within the itemstack or node.

@paramat
Copy link
Contributor

paramat commented Jun 26, 2020

PR is WIP but no progress for 4 months, is @lisacvuk still interested in continuing work?
Feature has core dev support in the feature request.

@sorcerykid
Copy link
Contributor

I would like to respectfully retract my earlier suggestion of using underscores for reserved fields in consideration of the fact that infotext and formspec are widely used already for node meta. Since no standard was picked early on, it's probably best not to introduce an more confusion by switching the naming scheme now.

My secondary impetus, however, is that I've been considering opening a PR to streamline private node meta by prefixing an underscore to the name, rather than having to use the somewhat obfuscated method mark_as_private and storing an extra boolean value in the database. So I feel that would be a better application of the underscore.

@lisacvuk
Copy link
Contributor Author

I will close this for now, as I don't have time (or interest) to work on it right now. Feel free to take over if you wish.

@lisacvuk lisacvuk closed this Jun 26, 2020
@velartrill
Copy link
Contributor

if there's still interest in this (read "it's likely to eventually be merged") i'd be willing to take it over

@rubenwardy
Copy link
Contributor

There's definitely interest in this feature, haven't checked this specific implementation though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature ✨ PRs that add or enhance a feature Possible close @ Script API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants