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

Update docs to allow non-liquid nodes to use "liquid" drawtype #14243

Merged
merged 1 commit into from Mar 24, 2024

Conversation

ryvnf
Copy link
Contributor

@ryvnf ryvnf commented Jan 10, 2024

Background

In Minetest, water is semi-opaque by default but it can be changed to fully opaque using the opaque_water setting.

In Mineclonia, we have semi-opaque ice. When a player has opaque_water enabled to save FPS on oceans, ice is still translucent which leads to significant FPS drops in frozen biomes.

We found a pretty neat solution for this, which is to make ice drawtype == "liquid". It makes ice follow the opacity setting for water. This works for us without any noticable problems.

Problem

Unfortunately, this appears to be forbidden according to the Minetest docs:

liquid_alternative_flowing = "",
liquid_alternative_source = "",
-- These fields may contain node names that represent the
-- flowing version (`liquid_alternative_flowing`) and
-- source version (`liquid_alternative_source`) of a liquid.
--
-- Specifically, these fields are required if any of these is true:
-- * `liquidtype ~= "none" or
-- * `drawtype == "liquid" or
-- * `drawtype == "flowingliquid"

It states that if drawtype == "liquid" then both liquid_alternative_flowing and liquid_alternative_source need to be specified. This is the case even if liquidtype ~= "none".

Solution

From testing, drawtype == "liquid" works with liquidtype == "none" even if the liquid alternative fields are not specified.

So this PR simply rewords the docs to make this conforming.

How to test

It can be tested that it works without engine modifications using this branch of Mineclonia. One can check this by placing a few ice blocks or do /findbiome ColdTaiga to teleport near naturally generated ice.

Last words

Using drawtype == "liquid" to make opacity togglable for a non-liquid node might seem like a hack, but I think it makes a lot of sense to support for ice which is basically frozen oceans.

The reason we do not want to use a game setting for this is because then it would be server-side and not client-side.

I also think there are other games would be interested in using this for translucent ice.

@sfan5
Copy link
Member

sfan5 commented Jan 12, 2024

IMO the opaque water setting is a good example of a too-specific setting hardcoded in the engine. It should rather be re-done.

@ryvnf
Copy link
Contributor Author

ryvnf commented Jan 12, 2024

IMO the opaque water setting is a good example of a too-specific setting hardcoded in the engine. It should rather be re-done.

"Leaves style" and "Connected glass" would also qualify then. This setting is just as specific as those.

I see your point but merging this has no implications on the future for these kinds of settings at all. This PR only changes the API documentation to make this officially supported when it already works.

The current specification also reads a bit contradictory:

liquidtype = "none",  -- specifies liquid flowing physics
-- * "none":    no liquid flowing physics
-- * "source":  spawns flowing liquid nodes at all 4 sides and below;
--              recommended drawtype: "liquid".
-- * "flowing": spawned from source, spawns more flowing liquid nodes
--              around it until `liquid_range` is reached;
--              will drain out without a source;
--              recommended drawtype: "flowingliquid".
-- If it's "source" or "flowing", then the
-- `liquid_alternative_*` fields _must_ be specified

This part suggests liquid_alternative_* does not need to be present if liquidtype == "none" but I guess it does not factor in the case when drawtype = "liquid".

In any case, I think I made a pretty good case for why it is a good idea to support this. I just want the opaque water setting to be able to work consistently with frozen biomes.

@ryvnf
Copy link
Contributor Author

ryvnf commented Jan 12, 2024

If someone knows why the API documentation was written like this please enlighten me about that. For me this works entirely without any issues for all for the Minetest versions I tested (5.9-dev, 5.8, 5.6).

I also think rejecting this without providing a good alternative would be a bad idea because then games will have to find ways to get the same behavior in non-sanctioned ways.

@sfan5
Copy link
Member

sfan5 commented Jan 13, 2024

probably @Wuzzy2 knows

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Jan 17, 2024

Hello! I was the one making the liquid decoupling a while ago and it looks like I had a brainfart here.
The goal was to decouple the graphical representation of "liquid-looking" nodes (i.e. the drawtypes) from actual gameplay behavior (flowing down, player movement physics, etc.). This goal has been achieved (with a few minor exceptions).

And yes, if only the liquid/flowingliquid drawtype is used and not anything else the liquid_alternative_* fields are actually optional.
What I meant to say is that these fields may affect rendering the nodes. You still can specify the liquid_alternative_* fields so that the nodes "connect" to each other properly.

Like, if you want to add a source/flowing liquid node pair but only for rendering purposes, it makes a lot of sense to add the liquid_alternative_*.
But if you only use one of those (source or flowing), you actually don't need the liquid_alternative_*.
Note this is only true if liquidtype=="none".

I think a singular drawtype="flowingliquid" node may not need a source variant either if it's liquidtype="none". But not 100%, please test to make sure.

Also, this note:

* You *must* set `liquid_alternative_source` to the node's own name unless
 `liquidtype == "none"`.

may actually be removed entirely because the only relevant case where you must set this is when liquidtype~="none" which is already covered in the node definition documentation.

BUT! I probably would instead add that the liquid_alternative_* fields are required for correct rendering if you use a drawtype="liquid" + drawtype="flowingliquid" node pair (because the renderer needs to know when to hide certain faces).

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Jan 17, 2024

To summarize:

  1. A node with drawtype="flowingliquid" ALWAYS MUST have a corresponding source node defined which is specified in liquid_alternative_source. Without that, there's a render bug. (This is more a limitation of liquid rendering right now and it would theoretically be possible to fix the rendering of this special case as well, making it official. But for now, this special case must be treated as "illegal" so it's technically not a bug)
  2. If drawtype=="liquid" and liquidtype=="none", then you can omit liquid_alternative_*
  3. liquid_alternative_* is always required if you intend to make a source/flowing node pair (no matter if render-only or if the liquid is also meant to flow)

@ryvnf
Copy link
Contributor Author

ryvnf commented Jan 17, 2024

Thanks for the information! I will update the PR later according to what you wrote.

@ryvnf
Copy link
Contributor Author

ryvnf commented Jan 18, 2024

I updated the branch after Wuzzy's comment. I think the docs is pretty clear about what is optional and not now.

Also thanks for decoupling the liquid rendering from its gameplay mechanics! It proved to be quite useful for us.

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

👁️ 👄 👁️ looks good to me, matches what Wuzzy said.

Also matches what the "liquid drawtype test nodes" (which work just fine) exist for in devtest.

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

whatever

@sfan5 sfan5 merged commit 20bfaba into minetest:master Mar 24, 2024
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

5 participants