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

air_equivalent is undocumented #13722

Closed
Wuzzy2 opened this issue Aug 7, 2023 · 16 comments
Closed

air_equivalent is undocumented #13722

Wuzzy2 opened this issue Aug 7, 2023 · 16 comments

Comments

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Aug 7, 2023

While looking through the builtin sources, I discovered the air node has a special field air_equivalent = true.

I don't know why it's there and this field is entirely undocumented. What's more confusing, it does not seem to be used anywhere in the code, neither in builtin nor C++ nor Minetest Game.

What is the purpose of this? If there is any purpose, I request that it'll be mentioned in lua_api.md.

@sfan5
Copy link
Member

sfan5 commented Aug 7, 2023

I know this used to be a node property back in (at least) 0.3.
If it's not used it should be removed.

@rollerozxa
Copy link
Member

It appears to be used in some games and mods on ContentDB: https://content.minetest.net/zipgrep/86162cee-f7ab-4495-9925-bef21efc19f9/

@Zughy
Copy link
Member

Zughy commented Aug 7, 2023

@rollerozxa this is my favourite part

image

I think it's safe to remove if it doesn't sort any effect

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Aug 8, 2023

I will not shed a single tear if this is removed. It was never documented so it's OK. If mods depend on undocumented behavior, it's their fault when things break.

@erlehmann
Copy link
Contributor

erlehmann commented Sep 11, 2023

This parameter was used internally to see if grass could grow underneath a node, i.e. if it propagated light.

Removing a node property that has been truthy for a long time (and thereby making it falsy, I guess) without figuring out the consequences seems either really stupid or really careless to me (choose your poison).

@rollerozxa
Copy link
Member

rollerozxa commented Sep 11, 2023

Since the previous zipgrep had expired, here's a new one, archived as to not expire: https://web.archive.org/web/20230911164208if_/https://content.minetest.net/zipgrep/501df1d6-4355-4fd8-8afb-947ea023aa6e/

Removing this might break NodeCore and some other packages if they don't change their behaviour.

@erlehmann
Copy link
Contributor

Removing this might break NodeCore and some other packages if they don't get fixed.

What “fix” do you have in mind? This property seems actually useful for me if you want to handle air, ignore and similar nodes correctly as, well ”air equivalent” in a game.

@rollerozxa
Copy link
Member

rollerozxa commented Sep 11, 2023

Removing this might break NodeCore and some other packages if they don't get fixed.

What “fix” do you have in mind? This property seems actually useful for me if you want to handle air, ignore and similar nodes correctly as, well ”air equivalent” in a game.

Well I was thinking checking for an airlike drawtype, but that would drag in other invisible nodes you might not want to be treated as an air equivalent so... Might be best to keep air_equivalent.

@erlehmann
Copy link
Contributor

Well I was thinking checking for an airlike drawtype

IMO this is most likely the wrong approach – think about it for a moment from a game/mod dev perspective …

@srifqi
Copy link
Member

srifqi commented Sep 12, 2023

The air_equivalent was first added in 4a6b9a6 (May 2011) for the engine and in d2eed16 (November 2011) for the builtin.

It was removed in c1479a2 (November 2011). The light_propagates and isLiquid() properties were used instead:

air_equivalent = light_propagates AND (NOT isLiquid())

(Note: There is light_propagates, but also there is sunlight_propagates.)

I am guessing that those two lines were missed when the air_equivalent was being removed since d2eed16 and c1479a2 are only a few hours apart and made by two different people.


Choices:

  • Remove it (since it is undocumented), but also announce the removal in the next release.
  • Mark it as deprecated and add a way to get a node's light propagation property and its liquid-ness in Lua.
  • Document it as a Lua-specific property (not used in the engine).

@rubenwardy
Copy link
Member

That makes sense, I was wondering if it was related to light/sunlight propagates

@erlehmann
Copy link
Contributor

IMO the easiest way to have no breakage is to leave it in and document it, ideally based on what mods actually do with it.

Mark it as deprecated and add a way to get a node's light propagation property and its liquid-ness in Lua.

What would be the actual advantage here of having every mod to build their air_equvalent themselves?

@erlehmann
Copy link
Contributor

erlehmann commented Oct 2, 2023

Note that while #13792 does mark air_equivalent as deprecated, it does not actually explain anything except “unclear meaning” and that it is set for air and ignore. It also does not propose an alternative to the 32 mods currently using that property.

For future developers who wish to remove air_equivalent: Make sure that you actually explain to mods what shoudl be used instead. So far, both “check for air and ignore” and “check for an airlike drawtype” seem to be obvious, but wrong answers to me.

Edit: @grorp suggested on IRC to use a custom node group and adding it via minetest.override_item and added “but there may be other alternatives, I don't know”.

@sfan5 sfan5 closed this as completed Oct 2, 2023
@sfan5
Copy link
Member

sfan5 commented Oct 2, 2023

So far, both “check for air and ignore” [...] seem to be obvious, but wrong answers to me.

Why?
It originally existed to declare under which nodes grass grows, this behavior is no more and it doesn't make sense for a game engine to dictate this.
Every modder who used this property was probably under the impression that it did something else.
The easiest way to keep the old behavior (regardless of whether it was used correctly or not) is to explicitly check for air or ignore.

@erlehmann
Copy link
Contributor

erlehmann commented Oct 2, 2023

So far, both “check for air and ignore” [...] seem to be obvious, but wrong answers to me.

Why?

Mods set air_equivalent for their own nodes. They do not set it for air or ignore – most likely because it was already set.

Piranesi restoration project for example does this: https://gitlab.com/search?search=air_equivalent&nav_source=navbar&project_id=41386041&group_id=128051&search_code=true&repository_ref=master

It originally existed to declare under which nodes grass grows, this behavior is no more and it doesn't make sense for a game engine to dictate this.

I fully agree with you here.

Every modder who used this property was probably under the impression that it did something else.

Sounds plausible, but I can not possibly know. Did you ask modders who used it what they used it for?

IIRC @Warr1024 elaborated on IRC a bit on using air_equivalent in NodeCore, what did you think about that?

The easiest way to keep the old behavior (regardless of whether it was used correctly or not) is to explicitly check for air or ignore.

This assumes that mods never set air_equivalent for their own nodes, which enough of them seem to do.

Since you came to a different conclusion than I did – at which mods did you look before deciding what to to?

@sfan5
Copy link
Member

sfan5 commented Oct 2, 2023

This assumes that mods never set air_equivalent for their own nodes, which enough of them seem to do.
Since you came to a different conclusion than I did – at which mods did you look before deciding what to to?

Zero.
If mods/games want to keep an informal standard for whatever air_equivalent is supposed to mean going that is their responsibility.

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

Successfully merging a pull request may close this issue.

7 participants