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

Nodebox: Allow nodeboxes to "connect" #3503

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@sofar
Member

sofar commented Dec 30, 2015

We introduce a new nodebox type "connected", and allow these
nodes to have optional nodeboxes that connect it to other
connecting nodeboxes.

This is all done at scenedraw time in the client. The client
will inspect the surrounding nodes and if they are to be connected
to, it will draw the appropriate connecting nodeboxes to make those
connections.

In the node_box definition, we have to specify separate nodeboxes
for each valid connection. This allows us to make nodes that connect
only horizontally (the common case) by providing optional nodeboxes
for +x, -x, +z, -z directions. Or this allows us to make wires
that can connect up and down, by providing nodeboxes that connect it
up and down (+y, -y) as well.

The optional nodeboxes can be arrays. They are named "connect_up",
"connect_down", and "connect_n|e|w|s".

Additionally, a "fixed" nodebox list present will always be drawn,
so one can make a central post, for instance. This "fixed" nodebox
can be omitted.

I've posted a screenshot demonstrating the flexibility at
screenie
In the screenshot, all connecting nodes are of this new subtype.

There are several things to note that are not yet in order:

Transparent textures render incorrectly, need tuning.

These nodes all connect to eachother. This needs to be a configurable
thing, so that one may opt to connect nodes based on a group, or
drawtype. This logic is not in place yet.

Example lua code needed to generate these nodes can be found here:
https://gist.github.com/sofar/b381c8c192c8e53e6062

Lastly, this requires a protocol bump as it needs to send the new
optional nodeboxes to the client in a structured method.

configurable connections

  • use reserve in boxes.
  • collisionboxes need to be updated as well
  • use default 0 more often to avoid passing in 0
  • fix up style copy-paste errors
  • figure out what nodes can actually connect to in the first place
  • protocol bump. Old clients won't be able to understand the new nodeboxes.
  • API change documentation
  • connect_sides parameter
@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Dec 30, 2015

Contributor

nice, I'll have a look at it until 2016.

Contributor

est31 commented Dec 30, 2015

nice, I'll have a look at it until 2016.

@BlockMen

This comment has been minimized.

Show comment
Hide comment
@BlockMen

BlockMen Dec 30, 2015

Contributor

What about the collisionboxes? Will they be handled the same way as the drawn nodeboxes and "added" on the fly?

In the current state it does look like a "connected" part of such a node has an incorrect collisionbox.

Contributor

BlockMen commented Dec 30, 2015

What about the collisionboxes? Will they be handled the same way as the drawn nodeboxes and "added" on the fly?

In the current state it does look like a "connected" part of such a node has an incorrect collisionbox.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Dec 30, 2015

Member

The Collision boxes will only contain the "fixed" nodeboxes currently. This is indeed still something that needs to be fixed.

Member

sofar commented Dec 30, 2015

The Collision boxes will only contain the "fixed" nodeboxes currently. This is indeed still something that needs to be fixed.

@est31

View changes

Show outdated Hide outdated src/mapnode.cpp
@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Dec 31, 2015

Contributor

Other than above comment and that code style demands space before parenthesis if before the parenthesis is a keyword (like if or while or so), that opening braces should be on the same line as the while/if/for statement, I can't find any further issues except those already pointed out by you.

Contributor

est31 commented Dec 31, 2015

Other than above comment and that code style demands space before parenthesis if before the parenthesis is a keyword (like if or while or so), that opening braces should be on the same line as the while/if/for statement, I can't find any further issues except those already pointed out by you.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Dec 31, 2015

Contributor

and of course lua_api.txt needs changes as well.

Contributor

est31 commented Dec 31, 2015

and of course lua_api.txt needs changes as well.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Dec 31, 2015

Member

@est31 yeah, that file has quite a few if( errors in them, I tried to avoid copying that :)

This patch obviously needs some more work, comments have been useful thus far. I'll revise in a few days as I usually take 2-3 days to wait for comments and resubmit.

Member

sofar commented Dec 31, 2015

@est31 yeah, that file has quite a few if( errors in them, I tried to avoid copying that :)

This patch obviously needs some more work, comments have been useful thus far. I'll revise in a few days as I usually take 2-3 days to wait for comments and resubmit.

@est31

View changes

Show outdated Hide outdated src/mapnode.cpp
@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Jan 2, 2016

Member

Added checkboxes to track comments and feedback integration in the PR up on top.

Member

sofar commented Jan 2, 2016

Added checkboxes to track comments and feedback integration in the PR up on top.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Jan 2, 2016

Member

As for the protocol version item: YES this does mean a protocol bump. Older clients connecting to a server with this code receives a "client to old to parse nodebox version".

Member

sofar commented Jan 2, 2016

As for the protocol version item: YES this does mean a protocol bump. Older clients connecting to a server with this code receives a "client to old to parse nodebox version".

@RobertZenz

This comment has been minimized.

Show comment
Hide comment
@RobertZenz

RobertZenz Jan 2, 2016

Contributor

Any chance you can extend this also for meshes?

Contributor

RobertZenz commented Jan 2, 2016

Any chance you can extend this also for meshes?

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Jan 2, 2016

Contributor

First things first, it can be added later. The more complex a pr, the longer it takes until its merged or gets disagreement.

Contributor

est31 commented Jan 2, 2016

First things first, it can be added later. The more complex a pr, the longer it takes until its merged or gets disagreement.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Jan 2, 2016

Contributor

Terminology note: "protocol bump" != "protocol break". Protocol break won't happen soon, as most of minetest clients use outdated versions.

Protocol bump is just about raising protocol versions, and that's no big deal.

For retaining compatibility you can for example provide a fallback for old clients, where only the fixed part of the nodebox is sent, as the normal nodebox drawtype. Or don't send the nodedef for the fixed nodebox, so that its undefined for the client. Both is better than denying old clients to join.

Contributor

est31 commented Jan 2, 2016

Terminology note: "protocol bump" != "protocol break". Protocol break won't happen soon, as most of minetest clients use outdated versions.

Protocol bump is just about raising protocol versions, and that's no big deal.

For retaining compatibility you can for example provide a fallback for old clients, where only the fixed part of the nodebox is sent, as the normal nodebox drawtype. Or don't send the nodedef for the fixed nodebox, so that its undefined for the client. Both is better than denying old clients to join.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Jan 3, 2016

Member

@est31 right, I think we can have old clients just show a regular full-sized node, since that's relatively easy, and would prevent possible abuse from missing collisionboxes.

Member

sofar commented Jan 3, 2016

@est31 right, I think we can have old clients just show a regular full-sized node, since that's relatively easy, and would prevent possible abuse from missing collisionboxes.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Jan 3, 2016

Member

@RobertZenz no, since the "mesh" specification doesn't allow this sort of thing to be implemented in the same fashion, unfortunately. The mesh drawtype requires a fixed model. It's always drawn in it's entirety.

Member

sofar commented Jan 3, 2016

@RobertZenz no, since the "mesh" specification doesn't allow this sort of thing to be implemented in the same fashion, unfortunately. The mesh drawtype requires a fixed model. It's always drawn in it's entirety.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Jan 3, 2016

Contributor

@sofar full sized node fallback is okay for me.

Contributor

est31 commented Jan 3, 2016

@sofar full sized node fallback is okay for me.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Jan 6, 2016

Member

I've added the needed protocol bump (27) and minor nodebox protocol bump (3). This allows older clients to connect and get meaningful nodebox data, and still play on servers at protocol 27.

Member

sofar commented Jan 6, 2016

I've added the needed protocol bump (27) and minor nodebox protocol bump (3). This allows older clients to connect and get meaningful nodebox data, and still play on servers at protocol 27.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Jan 6, 2016

Member

@est31 and indeed older clients get a proper full sized node so attempting to bypass fences will be impossible with older clients.

Member

sofar commented Jan 6, 2016

@est31 and indeed older clients get a proper full sized node so attempting to bypass fences will be impossible with older clients.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Jan 6, 2016

Contributor

other than that, I confirm, the only remaining issue looks like to be the missing system to connect to other nodes than those of the same type.

Contributor

est31 commented Jan 6, 2016

other than that, I confirm, the only remaining issue looks like to be the missing system to connect to other nodes than those of the same type.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Jan 6, 2016

Member

I can't think of anything else either. I may just want to do a PoC mesecons-like circuit to see if it behaves OK, but I know I have the uses cases in mind. It's just a matter of providing a solid and good usable API for making interconnecting devices and wires.

Member

sofar commented Jan 6, 2016

I can't think of anything else either. I may just want to do a PoC mesecons-like circuit to see if it behaves OK, but I know I have the uses cases in mind. It's just a matter of providing a solid and good usable API for making interconnecting devices and wires.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Jan 6, 2016

Contributor

docs are missing too.

Contributor

est31 commented Jan 6, 2016

docs are missing too.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Jan 7, 2016

Member

Added docs to the checklist in the github comment for tracking.

Member

sofar commented Jan 7, 2016

Added docs to the checklist in the github comment for tracking.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Jan 12, 2016

Member

Update, just so people know this is ongoing.

I'm adding a nodedef member connects_to = {} that allows you to specify nodes that this node needs to connect to. The server will match any group:wood type nodes to the actual nodes that are in that group.

This nodelist is then sent to each client when they connect, so clients only have to deal with the std::set connects_to_ids which should make for fast lookups.

So if I design a wooden fence, I could do something like:

connects_to = { "group:fence", "group:wood" }

Which would make my fence connect to fence nodes and full size wooden planks.

Member

sofar commented Jan 12, 2016

Update, just so people know this is ongoing.

I'm adding a nodedef member connects_to = {} that allows you to specify nodes that this node needs to connect to. The server will match any group:wood type nodes to the actual nodes that are in that group.

This nodelist is then sent to each client when they connect, so clients only have to deal with the std::set connects_to_ids which should make for fast lookups.

So if I design a wooden fence, I could do something like:

connects_to = { "group:fence", "group:wood" }

Which would make my fence connect to fence nodes and full size wooden planks.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar
Member

sofar commented Jan 13, 2016

working configurable connections

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Feb 27, 2016

Member

I'd like to see a connect_sides option added, so that this could be used for Technic wires and such.
Other than that, looks good.

Member

ShadowNinja commented Feb 27, 2016

I'd like to see a connect_sides option added, so that this could be used for Technic wires and such.
Other than that, looks good.

@C1ffisme

This comment has been minimized.

Show comment
Hide comment
@C1ffisme

C1ffisme Feb 27, 2016

@paramat I actually need this feature in my subgame to make lead and brass pipes.

C1ffisme commented Feb 27, 2016

@paramat I actually need this feature in my subgame to make lead and brass pipes.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 3, 2016

Member

@ShadowNinja I'd like that too (and I'll likely will be able to write that reasonably soon), but can we do that incrementally? I don't want to keep expanding this patch over and over with reasonable additions, but making the whole patch more and more complex.

Member

sofar commented Mar 3, 2016

@ShadowNinja I'd like that too (and I'll likely will be able to write that reasonably soon), but can we do that incrementally? I don't want to keep expanding this patch over and over with reasonable additions, but making the whole patch more and more complex.

@est31

View changes

Show outdated Hide outdated src/nodedef.cpp
@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 4, 2016

Member

Note we can tell to arbitrary connect only to selected sides.

Note the fence nodes only connect to the rear side of the chest.

Member

sofar commented Mar 4, 2016

Note we can tell to arbitrary connect only to selected sides.

Note the fence nodes only connect to the rear side of the chest.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 4, 2016

Member
  • fixed up one more repetition with a define
  • fixed up macro style (\t)
  • added connect_sides nodedef param for any node

Allows other nodes to tell to connected nodes that they can connect to the listed sides. Simple param2 rotation (n/e/w/s facedir) is properly calculated for, so you can e.g. connect a pipe to the back of a furnace as seen in the screenshot above. If this param is absent, all faces will connect of that node. You still have to list the node in the connects_to field in the connected nodedef, of course.

Member

sofar commented Mar 4, 2016

  • fixed up one more repetition with a define
  • fixed up macro style (\t)
  • added connect_sides nodedef param for any node

Allows other nodes to tell to connected nodes that they can connect to the listed sides. Simple param2 rotation (n/e/w/s facedir) is properly calculated for, so you can e.g. connect a pipe to the back of a furnace as seen in the screenshot above. If this param is absent, all faces will connect of that node. You still have to list the node in the connects_to field in the connected nodedef, of course.

@C1ffisme

This comment has been minimized.

Show comment
Hide comment
@C1ffisme

C1ffisme Mar 4, 2016

@sofar That is awesome. I would have never thought of adding that feature.

C1ffisme commented Mar 4, 2016

@sofar That is awesome. I would have never thought of adding that feature.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 5, 2016

Member

Updated doc to list all possible connect_sides

Member

sofar commented Mar 5, 2016

Updated doc to list all possible connect_sides

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 5, 2016

Member

There's a bug with the connect_sides code rotation handling. I'll check tonight.

Member

sofar commented Mar 5, 2016

There's a bug with the connect_sides code rotation handling. I'll check tonight.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 5, 2016

Member

Fixed up the rot[] table.

Member

sofar commented Mar 5, 2016

Fixed up the rot[] table.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 5, 2016

Member

Updated to allow front, back, left, right.

Member

sofar commented Mar 5, 2016

Updated to allow front, back, left, right.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 5, 2016

Member

Also added a warning if unrecognized side is specified through connect_sides

Member

sofar commented Mar 5, 2016

Also added a warning if unrecognized side is specified through connect_sides

@Ekdohibs

This comment has been minimized.

Show comment
Hide comment
@Ekdohibs

Ekdohibs Mar 6, 2016

Member

@sofar Why does connect_sides only work for simple facedir?

Member

Ekdohibs commented Mar 6, 2016

@sofar Why does connect_sides only work for simple facedir?

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 6, 2016

Member

Why does connect_sides only work for simple facedir?

because facedir is fundamentally incompatible with a bitwise face flag.

If implemented as a simple lookup table (like it is now), it would be 32(+1) * 24 large, and someone would have to calculate all the correct values. Right now the table is only 32(+1) * 4.

Member

sofar commented Mar 6, 2016

Why does connect_sides only work for simple facedir?

because facedir is fundamentally incompatible with a bitwise face flag.

If implemented as a simple lookup table (like it is now), it would be 32(+1) * 24 large, and someone would have to calculate all the correct values. Right now the table is only 32(+1) * 4.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Mar 6, 2016

Contributor

I think this is what faceposition cache is about, or am I wrong?

Contributor

est31 commented Mar 6, 2016

I think this is what faceposition cache is about, or am I wrong?

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 9, 2016

Member

@est31 maybe, but it's entirely unclear to me what it does at this point.

I'm sure there is a way to solve it, but I think the need for full 24x facedir support for this is likely extremely low. Additional patches can implement it, and, as before, I'll likely end up doing just that, but, not something I can do in a day right now.

Member

sofar commented Mar 9, 2016

@est31 maybe, but it's entirely unclear to me what it does at this point.

I'm sure there is a way to solve it, but I think the need for full 24x facedir support for this is likely extremely low. Additional patches can implement it, and, as before, I'll likely end up doing just that, but, not something I can do in a day right now.

@est31 est31 added the One approval label Mar 9, 2016

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Mar 9, 2016

Contributor

I think the patch already helps with the current behaviour, and can later on be improved. I've 👍 ed an earlier version above, and 👍 this version too.

Contributor

est31 commented Mar 9, 2016

I think the patch already helps with the current behaviour, and can later on be improved. I've 👍 ed an earlier version above, and 👍 this version too.

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 9, 2016

Member

@ShadowNinja @nerzhul ? Can we try and wrap this up for merge?

Member

sofar commented Mar 9, 2016

@ShadowNinja @nerzhul ? Can we try and wrap this up for merge?

@ShadowNinja

View changes

Show outdated Hide outdated doc/lua_api.txt

sofar added some commits Feb 25, 2016

Nodebox: Allow nodeboxes to "connect"
We introduce a new nodebox type "connected", and allow these nodes to
have optional nodeboxes that connect it to other connecting nodeboxes.

This is all done at scenedraw time in the client. The client will
inspect the surrounding nodes and if they are to be connected to,
it will draw the appropriate connecting nodeboxes to make those
connections.

In the node_box definition, we have to specify separate nodeboxes for
each valid connection. This allows us to make nodes that connect only
horizontally (the common case) by providing optional nodeboxes for +x,
-x, +z, -z directions. Or this allows us to make wires that can connect
up and down, by providing nodeboxes that connect it up and down (+y,
-y) as well.

The optional nodeboxes can be arrays. They are named "connect_top,
"connect_bottom", "connect_front", "connect_left", "connect_back" and
"connect_right". Here, "front" means the south facing side of the node
that has facedir = 0.

Additionally, a "fixed" nodebox list present will always be drawn,
so one can make a central post, for instance. This "fixed" nodebox
can be omitted, or it can be an array of nodeboxes.

Collision boxes are also updated in exactly the same fashion, which
allows you to walk over the upper extremities of the individual
node boxes, or stand really close to them. You can also walk up
node noxes that are small in height, all as expected, and unlike the
NDT_FENCELIKE nodes.

I've posted a screenshot demonstrating the flexibility at
    http://i.imgur.com/zaJq8jo.png
In the screenshot, all connecting nodes are of this new subtype.

Transparent textures render incorrectly, Which I don't think is
related to this text, as other nodeboxes also have issues with this.

A protocol bump is performed in order to be able to send older clients
a nodeblock that is usable for them. In order to avoid abuse of users
we send older clients a "full-size" node, so that it's impossible for
them to try and walk through a fence or wall that's created in this
fashion. This was tested with a pre-bump client connected against a
server running the new protocol.

These nodes connect to other nodes, and you can select which ones
those are by specifying node names (or group names) in the
connects_to string array:
      connects_to = { "group:fence", "default:wood" }
By default, nodes do not connect to anything, allowing you to create
nodes that always have to be paired in order to connect. lua_api.txt
is updated to reflect the extension to the node_box API.

Example lua code needed to generate these nodes can be found here:
    https://gist.github.com/sofar/b381c8c192c8e53e6062
Allow nodes to specify which sides to connect to.
NDT_CONNECTED attempts to connect to any side of nodes that it can
connect to, which is troublesome for FACEDIR type nodes that generally
may only have one usable face, and can be rotated.

We introduce a node parameter `connect_sides` that is valid for
any node type. If specified, it lists faces of the node (in "top",
"bottom", "front", "left", "back", "right", form, as array) that
connecting nodeboxes can connect to. "front" corresponds to the south
facing side of a node with facedir = 0.

If the node is rotatable using *simple* FACEDIR, then the attached
face is properly rotated before checking. This allows e.g. a chest
to be attached to only from the rear side.
@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Mar 12, 2016

Member
  • Converted all directions to "top", "bottom", "front", "left", "back", "right"
  • Recalibrated facedir rot table.
  • Re-tested with fences PR, walls PR and fencegate PR.
  • connect_sides now requires full word (e.g. "front"), not just a first letter. Prints warning if something else is found.
  • Adjusted docs to match all the changes.
Member

sofar commented Mar 12, 2016

  • Converted all directions to "top", "bottom", "front", "left", "back", "right"
  • Recalibrated facedir rot table.
  • Re-tested with fences PR, walls PR and fencegate PR.
  • connect_sides now requires full word (e.g. "front"), not just a first letter. Prints warning if something else is found.
  • Adjusted docs to match all the changes.
@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja
Member

ShadowNinja commented Mar 12, 2016

37b4f0d, e737b1c

paramat added a commit to minetest/minetest_game that referenced this pull request Mar 13, 2016

Convert fences to NDT_CONNECTED.
This changes the drawtype of fences to NDT_CONNECTED nodebox drawtype.

These nodes are drawn by the client with the needed connections on
the fly as the scene is drawn. There is no logic needed by mods to
modify the nodes.

These fences connect to (1) other fences, (2) planks and (3) tree
trunks, but nothing else. They do not connect to stone, dirt, wool,
etc. This is done by the "connects_to" parameter, which takes groups
and node names.

Due to the way textures are wrapped, we can make these nodes look a
lot better by giving them a special tile.

This change requires minetest/minetest#3503.

paramat added a commit to minetest/minetest_game that referenced this pull request Mar 13, 2016

Basic stone walls, using NDT_CONNECTED.
These basic connected wall nodes automatically connect
to neigboring stone blocks, other wall blocks and anything
that's "cracky". The do not connect to wood (fences will do
that).

The walls are generated using a new walls.register() API.
Documentation on the API is included in game_api.txt.

This change requires minetest/minetest#3503.

Walls are added for all cobble stone materials. They generally
look best and are the natural use cases for these materials.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment