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

Remove redundant minetest.register_node arguments #6246

Closed
wants to merge 2 commits into from

Conversation

HybridDog
Copy link
Contributor

Currently modders have to set paramtype to "light" manually when registering plants, else they become dark (nobody wants this). They also have to manually specify inventory_image and wield_image for specific drawtypes to avoid the unsightly default inventory item exhibition.
This PR makes setting those redundant, so registering plants, rails etc. becomes more convenient. You can still set paramtype to "none" manually for any reason.
The affected node drawtypes are documented in lua_api.txt that people still know when they e.g. can use param1 to store custom numbers.

@HybridDog
Copy link
Contributor Author

Any opinions?

@paramat
Copy link
Contributor

paramat commented Aug 20, 2017

+      Nodes with drawtype allfaces, allfaces_optional, torchlike, signlike,
+      plantlike, raillike, firelike, nodebox and mesh have paramtype set to
+      "light" if omitted

👎 This will be confusing because modders will need to memorise which nodes have the default set to "light" to know when to override it or not in the nodedef, currently it is easy to know what the default is because it is nil for everything.

@paramat
Copy link
Contributor

paramat commented Aug 20, 2017

-- Fix inventory_image and wield_image for specific drawtypes

Same issue here, it's much clearer for a nodedef behaviour to be the same for all drawtypes than differ for certain drawtypes.

@paramat
Copy link
Contributor

paramat commented Aug 20, 2017

Surely this would also break many mod nodes because the value of certain nodedefs when not specified has been changed.

@HybridDog
Copy link
Contributor Author

This will be confusing because modders will need to memorise which nodes have the default set to "light" to know when to override it or not in the nodedef, currently it is easy to know what the default is because it is nil for everything.

What do you mean by overriding it?
Do you mean, modders want to set paramtype to "none" although the plant becomes dark?

Same issue here, it's much clearer for a nodedef behaviour to be the same for all drawtypes than differ for certain drawtypes.

On the other hand, if the bad looking inventory image is fixed automatically, the nodedef table passed when registering the node looks more consistent for all drawtypes.

Surely this would also break many mod nodes because the value of certain nodedefs when not specified has been changed.

Can you give an example please?

@paramat
Copy link
Contributor

paramat commented Aug 21, 2017

Do you mean, modders want to set paramtype to "none" although the plant becomes dark?

Yes that may be wanted.

if the bad looking inventory image is fixed automatically, the nodedef table passed when registering the node looks more consistent for all drawtypes.

It may do, but that's a minor point.

Can you give an example please?

Examples are not needed. The fact is that nodedefs are created with some fields left blank because the default of that field is known, this PR changes the default behaviour so breaks nodedefs.

else they become dark (nobody wants this).

You can't make that assumption. Even though it may be rare, this is still breakage, and more importantly, confusing because modders now have to memorise changing default behaviour dependent on ndrawtype.

They also have to manually specify inventory_image and wield_image for specific drawtypes to avoid the unsightly default inventory item exhibition.

Same problems.

So this is extra code that breaks nodes and makes things more confusing.

@paramat
Copy link
Contributor

paramat commented Aug 21, 2017

This only automatically fixes things that will be seen and fixed by the modder anyway, it's better they realise their mistakes than having code cover up their mistakes.

@HybridDog
Copy link
Contributor Author

I don't think these are mistakes of modders. The inventory items look buggy because in minetest core the inventory images aren't set properly for some drawtypes.
In my experience it's a lot more annoying to memorize for which drawtypes I explicitly need to set paramtype to light and fix inventory images although l actually only want to avoid black looking plants and strange inventory images.
In future this behaviour may be changed, so l may have to manually remove paramtype = "light" from the nodedefs for better light calculation.

l tried to make this not cause compatibility problems. Automatically fixing inventory and wield images and setting paramtype to light when it's nil to fix dark nodes (lua_api tells which drawtypes are affected) shouldn't break mods.

@paramat
Copy link
Contributor

paramat commented Aug 22, 2017

The inventory items look buggy because in minetest core the inventory images aren't set properly for some drawtypes.

It's the responsibility of the modder to set nodedefs correctly.

In my experience it's a lot more annoying to memorize for which drawtypes I explicitly need to set paramtype to light and fix inventory images

These don't need memorising, it's obvious what nodedefs to use. It's better that modders understand how nodedefs work than to leave nodedefs blank and let the engine choose the defaults.
Any mistake the modder makes will be immediately obvious as soon as they test, and then they learn ffrom it.

In future this behaviour may be changed

It won't because we try not to break nodedefs, which is what this PR will do by altering defaults.

@HybridDog
Copy link
Contributor Author

It's better that modders understand how nodedefs work than to leave nodedefs blank and let the engine choose the defaults.

l think you're right. Modders then also understand easier how the minetest engine works, e.g. how light is propagated in minetest.

How about adding a separate function? When calling it for registering a node you need to specify only a few things in the nodedef, e.g. registering a plant, when more features are added (remember the waving field), they are added to that node automatically if the modder didn't explicitly disable it.

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

2 participants