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

Add drawtype sunken and covered #14103

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

sfence
Copy link
Contributor

@sfence sfence commented Dec 14, 2023

Add new drawtypes sunken and covered.

To do

This PR is a Work in Progress.

  • Documentation in lua_doc.md
  • More examples
  • Update transformLiquids logic to prevent overwriting of the sunken node by alternative_source
  • Rendering logic for liquids and sunken node updated.
  • Change param2 usage in covered drawtype for more flexibility.
  • Add lua script check to prevent setting another sunken or covered drawtype node as inner_node of sunken/covered node to prevent the possibility of infinite rendering loop.

How to test

Run game devtest and place added test nodes testnodes: testnodes:sunken_torchlike, testnodes:sunken_nodebox, testnodes:sunken_mesh, testnodes:covered_torchlike_node, testnodes:covered_nodebox_node and/or testnodes:covered_meshnode.
You can change param2 of testnodes:covered_torchlike to simulate the uncovering process.

screenshot_20231214_153449
screenshot_20231214_164317

@cx384
Copy link
Contributor

cx384 commented Dec 14, 2023

I don't think those new drawtypes are particularly bad, but it would be good to have a discussion first, how useful they are and some use cases.
plantlike_rooted for example already works in water and you can use special_tiles to make it look similar to your covered drawtype.

@sfan5 sfan5 added @ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Dec 14, 2023
@sfence
Copy link
Contributor Author

sfence commented Dec 14, 2023

I don't think those new drawtypes are particularly bad, but it would be good to have a discussion first, how useful they are and some use cases. plantlike_rooted for example already works in water and you can use special_tiles to make it look similar to your covered drawtype.

I know about planlike_rooted, but how it can be used to do sunken two-node height doors on shipwrecks for example? Doors typically included the main node and the invisible node above. And what about the sunken trap door?
Can I use plantlike_rooted to make nodebox or mesh-based node look like covered or sunken?

Or can be special_tiles used to show, particularly covered nodes?

I have to add more examples. :)

@sfence
Copy link
Contributor Author

sfence commented Dec 14, 2023

@cx384 So I have added a new screenshot to show how it works with testnodes:nodebox_fixed node.

@cx384
Copy link
Contributor

cx384 commented Dec 14, 2023

I'm still not very convinced.
For the nodebox inside a node covered it may be better to just have a drawtype that allows multiple nodeboxes inside one node (with different textures).

Or add the possibility for node boxes to use special_tiles instead of the covered type.

For liquids your PR may be able to fix #3074.

@sfence
Copy link
Contributor Author

sfence commented Dec 14, 2023

@cx384 It can be an interesting update for nodebox nodes. Some solution like it comes to my mind too. But I think this solution is more flexible.

I only include "link" to another node a draw two nodes in one position.

I think you can't do this with nodebox at all: screenshot_20231214_164317 (it is all "testnodes:covered_mesth_node")

So, you can use it with mesh, plant, nodebox, torchlike, etc nodes. And probably whatever type of node that will be added in the future.

@wsor4035
Copy link
Contributor

just popping in to say plantlike_rooted is a hack that sorta gets you suken/covered nodes, as it requires you to have the node underneath accessible/or extend your node very long (see kelp), and then add additional logic to to handle breaking "per node". also it requires you to have multiple variations if you want different types of nodes underneath(sand/gravel/dirt/etc for example)

@cx384
Copy link
Contributor

cx384 commented Dec 14, 2023

covered may not be necessary for mesh, since you can incorporate the cover into the mesh, but it is still useful.

A use case I can think of is a sapling covered in snow.

@wsor4035
Copy link
Contributor

covered may not be necessary for mesh, since you can incorporate the cover into the mesh, but it is still useful.

A use case I can think of is a sapling covered in snow.

unless im misunderstanding something here, to do this currently you would have to make #levels meshes/nodes (cutting into the 32k limit) currently since the covered type means the height updates with param2

@MisterE123
Copy link
Contributor

This is extremely useful, if it solves the problem it claims to - doors and other items creating air pockets in Mesecraft is very annoying and also buggy- its possible to breathe underwater for as long as you like by placing some node that creates an airpocket (like a slab). A conceptual issue is, will this allow sunken nodes to be mesh or nodebox drawtypes?

@Lemente
Copy link
Contributor

Lemente commented Dec 14, 2023

If I understand correctly, you could superimpose any node?

You could just have two superimposed water nodes for example?
and the entites would be affected by both of their properties? Would they be cumulated? So twice as slow for two water nodes?

Do you have to register a different node for it to be sunken in different other nodes? door in water, door in river water, door in lava
Could flowing water seamlessly flow through nodes? Could it stop or flow depending on the door/trapdoor position?

And I assume lava would have to check for water and the sunken drawtype to work again, right?

If the node is sunken in lava, I'm assuming the lava ABM would not be used, right? Which means every node that could be sunken in lava would need an ABM that checks if it is currently sunken in lava (or any other liquid with an ABM), and trigger that ABM if so.

@sfence
Copy link
Contributor Author

sfence commented Dec 15, 2023

@wsor4035 Yes, I think you get it.
In this proof on-concept version I am using the top 4 bits in param2 to calculate covered node height.

@sfence
Copy link
Contributor Author

sfence commented Dec 15, 2023

@MisterE123 Yes, it is a target for sunken drawtype.
I have to fix rendering on contact with the original liquid and sunken node. Node sides should not be rendered here to make it look well.
This problem is connected with liquids updating logic in the transformLiquids method via liquid_alternatives files in node definition. So I have to find some nice way to fix it with existing node definition fields or add some field.

@sfence
Copy link
Contributor Author

sfence commented Dec 15, 2023

@Lemente

If I understand correctly, you could superimpose any node?

Yes, this solution can be applied to any other registered node.

You could just have two superimposed water nodes for example? and the entites would be affected by both of their properties? Would they be cumulated? So twice as slow for two water nodes?

You can apply it to another liquid node as well, but you have to keep in mind, that it is only a rendering feature. So any special logic for mixed liquid nodes will have to be scripted. For the server only properties of the parent node are relevant.

Do you have to register a different node for it to be sunken in different other nodes? door in water, door in river water, door in lava Could flowing water seamlessly flow through nodes? Could it stop or flow depending on the door/trapdoor position?

Yes, you have to register a node for every possible combination of sunken node and liquid.
I do not see a way to go around this without using metadata for rendering or something similar. However, the use of metadata for rendering will probably have a bad effect on rendering performance.

I am working with two solutions in my mind.

  1. Simply keep in the hands of ABMs and Lua callbacks.
  2. Try to do some integration with the actual liquid updating engine - maybe this can be done in another pull request later if this pull request is approved.

And I assume lava would have to check for water and the sunken drawtype to work again, right?

I think this is solved by lava ABM looking for nodes in group:water. So simply adding node sunken in water to that group should be enough.

If the node is sunken in lava, I'm assuming the lava ABM would not be used, right? Which means every node that could be sunken in lava would need an ABM that checks if it is currently sunken in lava (or any other liquid with an ABM), and trigger that ABM if so.

It depends on ABM's definition. If ABM works only for node default:lava_soruce for example, it will not work. If ABM is defined to work for all nodes in group group:lava, it can be enough to add a sunken node into this group. But depending on the situation, some changes in ABM logic can be required too.

@sfence
Copy link
Contributor Author

sfence commented Dec 16, 2023

Commit e73b190 fix liquid source replacing iwth alternative liquid source. This is also related to problem mentioned here minetest-mods/wielded_light#4 by @bell07. So if this will be merged, wielded_light mod liquid sources can be updated to be better integrated.

This also allows sunken node to generate flowing liquid of the wanted type. This can be also used in wielded_light.

@sfence
Copy link
Contributor Author

sfence commented Dec 16, 2023

screenshot_20231216_110559
screenshot_20231217_131933

So, the sunken nodes now look natural in the associated liquid.

@tigercoding56
Copy link

could this be used for waterlogged nodes ? , if so is it possible to have a filter for where to cover a node (so we could waterlog only 1 side of glass pane for instance)

@sfence
Copy link
Contributor Author

sfence commented Jan 3, 2024

could this be used for waterlogged nodes ? , if so is it possible to have a filter for where to cover a node (so we could waterlog only 1 side of glass pane for instance)

Liquid nodes are rendered as solid nodes by the engine. So, it means that you can use different texture for each side and the node should be rotable with param2.
So, it should be possible to realize it in a way, which will look like a glass pane with water on one side.

It will be a good idea to set liquid_range to 0 and liquid_renewable to false for nodes like this.

@Zughy
Copy link
Member

Zughy commented Jan 14, 2024

So, this PR is related to a "Supported by core dev" issue, but

  1. it looks like it adds more than that
  2. the label was put by a former core dev, who left the organisation years ago

For the aforementioned reasons, I'll leave the choice about what to do with this PR to core devs in the next meeting => https://dev.minetest.net/Meetings#2024-01-21

@Desour
Copy link
Member

Desour commented Jan 16, 2024

It's quite hard to judge if I support this if there's no detailed description of what this actually does. Doc draft needed.

@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 16, 2024
@sfence
Copy link
Contributor Author

sfence commented Jan 17, 2024

@Desour I added some info to lua_doc.md, but screenshots are probably still better to show what it does than a thousand words.

If you have any additional questions, feel free to ask.

@Desour
Copy link
Member

Desour commented Jan 17, 2024

Ah, I see now. So for sunken nodes, you need to register a new node for each liquid x inner node pair. And this doesn't work if the inner node needs param2, I suppose? It's obviously an imperfect solution. Idk if it is better than nothing.

The covered drawtype with level looks interesting to me for dust covered plants (like the snow sapling mentioned before).

=> I'm unsure and will wait for more opinions first.

@sfence
Copy link
Contributor Author

sfence commented Jan 17, 2024

Ah, I see now. So for sunken nodes, you need to register a new node for each liquid x inner node pair. And this doesn't work if the inner node needs param2, I suppose? It's obviously an imperfect solution. Idk if it is better than nothing.

The covered drawtype with level looks interesting to me for dust covered plants (like the snow sapling mentioned before).

=> I'm unsure and will wait for more opinions first.

In the actual implementation of the covered draw type I use param2 & 0xF0 for level and param2 & 0x0F value is used by the inner node. So it is not ideal, but still useful for some nodes. Sunken drawtype does not use param2, so full param2 is available for the inner node now.

Because param1 is needed for light, the only different way how to implement it is probably to use metadata. However, using metadata for rendering maps does not sound like a great idea because of its performance. On the opposite side, with metadata, inner_node can be specified from metadata as well.

If it is requested, I will make the same changes to do it in the preferred way.

@sfence
Copy link
Contributor Author

sfence commented Jan 17, 2024

New ideas in my mind about using param2 and field inner_node:

It can be determined by leveled_max value, how many bites can be used for the inner node.

And it will not be probably complicated to implement some loaded metadata cache based on unordered_map for example, to prevent accessing node metadata on every scene rendering. So, it should fix potential performance problems and sounds like a preferred solution to me.

@SmallJoker
Copy link
Member

SmallJoker commented Jan 21, 2024

Discussed in the meeting: https://irc.minetest.net/minetest-dev/2024-01-21#i_6147292

  1. The main goal appears to be to submerge nodes under liquids. This requires to register each node combination again - which is very inefficient.
    • Proposed solution: introduce a node def field to indicate whether a node may be submerged (e.g. submerge = "none|liquids|custom"). The on_flood` callback also exists to implement special rules in Lua (e.g. replacing the node in certain liquids)
    • Merging two nodes cannot be seamless. There's only param2, thus limited possibilities. I did not check the code yet but I'd guess that you could not have a liquid level + node rotation at the same time. An additional submerge field would allow more possibilities.
  2. Observation: Some of the functionality appears to be similar in concept to glasslikeliquidlevel.
  3. The code is currently not very readable and possibly duplicated. Needs proper reviewing to provide suggestions.

If there are any questions, please reach out.

@sfence
Copy link
Contributor Author

sfence commented Jan 25, 2024

The new version removed the need for node pair registration for visual purposes. Inner nodes are now defined by metadata fields inner_node and inner_param2. These fields are cached, so rendering does not need to access the metadata class. I believe that this solution can be expanded in the future for different purposes.

Would be nice if someone checked if this solution looks acceptable.

@SmallJoker I am not sure about submerge suggestion. Looks like this check if the node can be flooded can be done by on_flood callback. Similarly, if the node can be covered, it will require some custom Lua code to do so, so a check can be done in it. The server probably does not need to know if the node can be submerged.
Or should I check in render logic the submerged node submerge field and render it only if the submerge field is in the expected value?

@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 21, 2024
@Zughy
Copy link
Member

Zughy commented Feb 21, 2024

@SmallJoker reminder

@SmallJoker
Copy link
Member

SmallJoker commented Feb 24, 2024

@sfence floodable/on_flood describes the "physical" behaviour when nodes come in touch with liquid. What I meant with submerge is to only adapt the visuals to the surrounding liquid (for now, player physics are a different issue). This way, plants or any other node could be placed underwater without worrying about air gaps like seen here:

air gap of dry shrub

Getting a decent underwater appearance would then be pretty trivial for mods to enable. One nodedef field.

@sfence
Copy link
Contributor Author

sfence commented Feb 24, 2024

@SmallJoker
I do not think, that the submerge field will be enough to work in situations like this:

screenshot_20240224_100556
screenshot_20240224_100759

@cx384 cx384 mentioned this pull request Mar 20, 2024
@nininik0
Copy link

What version of devtest is this?

@sfence
Copy link
Contributor Author

sfence commented Mar 23, 2024

What version of devtest is this?

This is an unmerged branch without the core developer's support.
To try it, you must clone this branch and compile Minetest.

But at the moment, it looks like it will not be supported and merged.

@AlexKurisu
Copy link

IMHO, this whole thing should be a part of a more generic multipart-like API

@sfence
Copy link
Contributor Author

sfence commented Apr 4, 2024

IMHO, this whole thing should be a part of a more generic multipart-like API

Maybe, something like #13811 can be used for your idea, after some extension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet