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

fix setmetatable for builtin:item enhancement #2286

Closed
wants to merge 2 commits into from
Closed

fix setmetatable for builtin:item enhancement #2286

wants to merge 2 commits into from

Conversation

bell07
Copy link
Contributor

@bell07 bell07 commented Jan 10, 2019

The reason for this fix is the wrong metatables usage that take nil values for some conditions. I had similar issues in my smart_inventory too, reported in https://forum.minetest.net/viewtopic.php?t=19355#p330608 and fixes in minetest-mods/smart_inventory@d2328eb

[outdated, was first "no new table for item_entity enhancement" commit]
But for item_entity there is no reason to create new lua table for builtin item, just enhance the existing one is enough. Therefore this PR removes the new table creation and just enhances the original lua-table. This way no setmetatable and no re-registration needed.
Small additional change is the syntax change to OO-Style for enhanced functions

@bell07
Copy link
Contributor Author

bell07 commented Jan 10, 2019

I see, travis say me the
mods/default/item_entity.lua:6:10: indirectly setting read-only field 'registered_entities.__builtin:item.set_item' of global 'minetest'

Now I create new fix just for setmetadata

@bell07 bell07 changed the title no new table for item_entity enhancement fix setmetatable for builtin:item enhancement Jan 10, 2019
@paramat paramat added the Bugfix label Jan 11, 2019
@bell07
Copy link
Contributor Author

bell07 commented Jan 15, 2019

found out the builtin_item does have __index entity from any place that points back to builtin_item. That does work but bad code practice from my point of view. The meta methods like __index should not be mixed in the same table with other methods

@paramat
Copy link
Contributor

paramat commented Jan 18, 2019

It would be nice to merge this for 5.0.0, note i'm clueless when it comes to metadata so can't really review this.

@paramat
Copy link
Contributor

paramat commented Feb 8, 2019

@sofar could you review?

@paramat
Copy link
Contributor

paramat commented Mar 6, 2019

Replaced by #2328

@paramat paramat closed this Mar 6, 2019
@bell07
Copy link
Contributor Author

bell07 commented Mar 6, 2019

Tested, works as expected with wielded_light

@bell07 bell07 deleted the item_entity_fix branch April 10, 2019 17:16
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.

2 participants