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

Disable "Ignoring CONTENT_IGNORE redefinition" warning #4393

Merged
merged 1 commit into from Feb 18, 2018

Conversation

HybridDog
Copy link
Contributor

@HybridDog HybridDog commented Jul 31, 2016

Addresses #6276

@HybridDog HybridDog changed the title Disable "Ignoring CONTENT_IGNORE redefinition" warning Disable "Ignoring CONTENT_IGNORE redefinition" warning caused by builtin Jul 31, 2016
@@ -303,7 +303,7 @@ core.register_node(":air", {
groups = {not_in_creative_inventory=1},
})

core.register_node(":ignore", {
core.registered_nodes["ignore"] = {
Copy link
Contributor

@t4im t4im Jul 31, 2016

Choose a reason for hiding this comment

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

Will need some fields more, if you do it without register_node.

    name = "ignore",
    mod_origin = core.get_current_modname()

You probably should also have its metatable __index point to core.nodedef_default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@t4im fixed

@HybridDog HybridDog closed this Sep 1, 2016
@HybridDog HybridDog reopened this Sep 17, 2016
@HybridDog HybridDog changed the title Disable "Ignoring CONTENT_IGNORE redefinition" warning caused by builtin Disable "Ignoring CONTENT_IGNORE redefinition" warning by adding a register_raw field to register_item Sep 17, 2016
@sofar
Copy link
Contributor

sofar commented Feb 10, 2017

OK for me, it kind of makes sense? 👍. I can see this being useful for other things like hand, air and a few more things.

@paramat
Copy link
Contributor

paramat commented Mar 26, 2017

Build problems?

@SmallJoker
Copy link
Member

Looks good but I would prefer another term for easier understanding. How about is_core_content or is_predefined?

@HybridDog
Copy link
Contributor Author

l agree; l called it pass_to_core.

@paramat it disappeared after rebasing

@rubenwardy
Copy link
Member

rubenwardy commented Aug 11, 2017

I'm not completely sure about this. Is this because air and ignore can't be redefined?

@nerzhul
Copy link
Member

nerzhul commented Aug 11, 2017

it seems a little bit hacky for a single node, maybe we should just merge contentfeatures ?

@HybridDog
Copy link
Contributor Author

When making mods you can use this to e.g. add the on_blast function to some node definition without redundantly passing everything to minetest core.

@nerzhul
Copy link
Member

nerzhul commented Aug 18, 2017 via email

@paramat
Copy link
Contributor

paramat commented Aug 18, 2017

Context: I deleted my comment, that was just before nerzhul's, where i suggested moving the message to infostream.

@HybridDog
Copy link
Contributor Author

Could anybody donate a second approval?

@paramat
Copy link
Contributor

paramat commented Aug 19, 2017

How about just disabling the warning for the ignore node only, to keep it simple?

@HybridDog
Copy link
Contributor Author

Do you mean explicitly avoiding printing this warning when the node is CONTENT_IGNORE?

@paramat
Copy link
Contributor

paramat commented Aug 19, 2017

Yes, but it's just a suggestion, it might not be a good idea.

@HybridDog
Copy link
Contributor Author

HybridDog commented Aug 20, 2017

l think that would be a workaround. The "ignore" node is overriden in register.lua that mods which use node definitions (e.g. minetest.registered_nodes[minetest.get_node(pos).name]) can easily treat ignore like other nodes, e.g. test whether it's walkable.
Another solution is setting minetest.registered_nodes["ignore"] to nil, ignore would be treated like unknown nodes. But doing this may add compatibility problems.
So l think adding a pass_to_core field to the nodedef for registering is a more appropriate solution.

@HybridDog
Copy link
Contributor Author

HybridDog commented Aug 28, 2017

l could also temporarily override register_item_raw to an empty function when reregistering ignore. How do you think about it?

@rubenwardy
Copy link
Member

rubenwardy commented Dec 17, 2017

I don't like this, it feels hacky and a misfeature just to prevent this warning. It feels like a lot of ugliness for a small issue. I suggest:

  1. remove C++ definition of ignore, and give canonical definition in builtin. Then disallow any overriding.
  2. make not registering core something that only happens to ignore

Number 2 sounds better to me.

It would be:

function core.register_item(name, itemdef)
    local is_overriding = core.registered_items[name]


    -- rest of function


    -- Don't register ignore with the engine
    if name ~= "ignore" then
        register_item_raw(name, def)
    elseif is_overriding then
        minetest.log("warning", "Ignoring CONTENT_IGNORE redefinition")
    end
end

@HybridDog
Copy link
Contributor Author

Done

@paramat paramat removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 13, 2018
@paramat
Copy link
Contributor

paramat commented Feb 13, 2018

Thanks, no objections from me.
@rubenwardy

@paramat
Copy link
Contributor

paramat commented Feb 14, 2018

👍

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Feb 14, 2018

Hmmm. I'm not sure if I understood this right.

Does this always disable the warning, or just when ignore is added by builtin?

IMO the warning does make sense when you try to overwrite ignore from a mod that is not builtin, so it should only be hidden when builtin tries to set it.

@rubenwardy
Copy link
Member

That's what will happen

if name ~= "ignore" then
register_item_raw(itemdef)
elseif is_overriding then
minetest.log("warning", "Attempted redefinition of \"ignore\"")
Copy link
Contributor

Choose a reason for hiding this comment

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

use the core namespace in builtin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do.

Copy link
Contributor Author

@HybridDog HybridDog Feb 16, 2018

Choose a reason for hiding this comment

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

Done,
l also changed the comment because avoiding mods to register ignore to core is just a redundant side effect.

minetest.override_item still passes to core
@paramat
Copy link
Contributor

paramat commented Feb 16, 2018

👍

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Feb 17, 2018

@rubenwardy: You answer was not hellpful. :-(
Now what will happen?

@HybridDog
Copy link
Contributor Author

HybridDog commented Feb 17, 2018

It avoids registering ignore to core if it's registered the first time in lua, i.e. in builtin.
As side effect, for subsequent ignore registrations it gives a warning instead of attempting to register to core (which would also only give a warning, just from the core side).

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Feb 17, 2018

Ah, good. That's what I wanted to hear. :-)

@SmallJoker SmallJoker merged commit 46bbace into minetest:master Feb 18, 2018
@HybridDog HybridDog deleted the patch-2 branch February 18, 2018 12:05
minduser00 pushed a commit to minduser00/minetest that referenced this pull request Mar 18, 2018
minduser00 pushed a commit to minduser00/minetest that referenced this pull request Mar 18, 2018
sfan5 pushed a commit that referenced this pull request Apr 21, 2018
minetest.override_item still passes to core
@sfan5 sfan5 mentioned this pull request Apr 21, 2018
sfan5 pushed a commit that referenced this pull request May 13, 2018
minetest.override_item still passes to core
HybridDog added a commit to HybridDog/minetest that referenced this pull request May 13, 2018
Revert "Fix "Ignoring CONTENT_IGNORE redefinition" warning (minetest#4393)"

This reverts commit 46bbace.

Then do the ignore tests in l_item.cpp instead of builtin/
sfan5 pushed a commit that referenced this pull request May 13, 2018
minetest.override_item still passes to core
HybridDog added a commit to HybridDog/minetest that referenced this pull request May 18, 2018
Revert "Fix "Ignoring CONTENT_IGNORE redefinition" warning (minetest#4393)"

This reverts commit 46bbace.

Then do the ignore tests in l_item.cpp instead of builtin/
SmallJoker pushed a commit that referenced this pull request Jun 3, 2018
minetest.override_item still passes to core
osjc pushed a commit to osjc/minetest that referenced this pull request Jan 11, 2019
osjc pushed a commit to osjc/minetest that referenced this pull request Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants