3d line rendering.

[Andrey2470T]

This PR is particularly based on the closed #10619 and introduces an early implementation of rendering dynamic 3d lines for various goals. Currently, it is possible to add/change positions and colors of both ends, diameter, attaching to objects, clamp mode, texture, backface culling, animation and light level.

Next visual types are supported:

• sprite: always facing to the player.
• flat: represents itself 3d rectangle.
• dihedral: two rectangles intersecting orthogonally to each other.
• cylindric: represents a cylinder whose rotation axis goes from the start point to the end one.

To do

This PR is Ready for Review.

How to test

1. Download the test mod:
   line_test.zip
2. Enter /add_3dline to add new line with some default properties.
3. Then enter /change_3dlines to randomly change the properties of the current added lines (type, colors, width and texture).
4. If you don't need the lines anymore, summon /remove_3dlines that will remove all lines.

[ghost]

Does this allow for ropes such as fishing lines in Minecraft? Or is that a different thing?

[SmallJoker]

Is there a particular reason or advantage why the 3D lines use a whole new concept rather than re-using and extending existing ActiveObject functionalities (e.g. to add a "tiling" option)?

[Andrey2470T]

Does this allow for ropes such as fishing lines in Minecraft? Or is that a different thing?

Yes, my PR does it quite possible.

Is there a particular reason or advantage why the 3D lines use a whole new concept rather than re-using and extending existing ActiveObject functionalities (e.g. to add a "tiling" option)?

If you hint adding a new visual type of objectref, I'll explain why. The main reason is the lines are defined by two positions in space and as a consequnce have two colors and can be attached to two entities/players simultaneously. That's the first.

Secondly, the most methods as set_pos, set_velocity, set_acceleration and especially set_hp, punch, set_armor_groups and other would be useless as minimum because of all properties are set via change_properties by the only call. The lines can't have velocity/acceleration since being movable is not their assignment. Also, having HP would be absurd. Well, as a consequence the armor groups wouldn't also make sense. So, I decided to bring the implementation in a separate abstraction.

[ghost]

cool to make dog leashs

[ghost]

it should be cool a method to get the line lenght.

[appgurueu]

General code-wise stuff:

• You're duplicating some code for the two line ends (e.g. for the light level calculation or the position updates). Deduplicate it using private / static helper functions, anonymous functions / lambdas, or macros (only as a last resort if for some reason functions don't work well).
• You still have outcommented code in there. Get rid of it.
• You seem to have almost no comments in the code. Add comments please.
• Your big switch for line params could probably replaced using a map.

Concept-wise:

Lines are practically global - that is, there is one global set of lines referenced by ID. Line management is entirely left up to the modder; there is no mechanism in place to "unload" inactive lines. I'm fairly certain that if this was given to modders as a tool, the first thing you'd get would be memleaks as modders forget to properly remove their lines.

Garbage-collecting a line should perhaps also remove it.

Particularly problematic: All lines are sent to all clients. This is both bad for performance (both server & network load) and confidentiality: Suddenly each client knows when a line is added on the other edge of the map. Using modified clients, this may be exploited to find hideouts of other players through line-creating activity.

Ideally, lines should use the same logic as entities, where the server only sends entities in a reasonable range.

Disclaimer: I am not a core dev.

[Andrey2470T]

• Your big switch for line params could probably replaced using a map.

Hmm. But the line params is a structure and contains different data types and the map container can contain the only one. What did you actually suggest?

there is no mechanism in place to "unload" inactive lines.

To be honest, whether really is it necessary to be? The lines behave similar to the particlespawners -- they are added to the clientmap independently from where the localplayer locates and get hidden if the distance from the player to them exceedes the max block send distance. Also, they are not saved after the game exit.

Garbage-collecting a line should perhaps also remove it.

There's __gc metamethod for the lua3dline object. So I'm sure it should be collected when it's not referenced to anymore.

Particularly problematic: All lines are sent to all clients. This is both bad for performance (both server & network load) and confidentiality: Suddenly each client knows when a line is added on the other edge of the map. Using modified clients, this may be exploited to find hideouts of other players through line-creating activity.

Well, they are sent only in certain cases -- when added, their properties are changed and removed. Why would it burden the network?

[Andrey2470T]

Does anybody have an idea why does it fail to be linked on the android version? It reports about unknown symbols like LineParams::reset(), LineParams::serialize() and LineParams::deserialize() although the line_params.cpp where they are defined should be seen by the linker, e.g.:

ld: error: undefined symbol: LineParams::reset() referenced by line_params.h:60 (../../src/line_params.h:60) /home/runner/work/minetest/minetest/android/native/build/intermediates/cxx/Release/6m4z1o42/obj/local/arm64-v8a/objs/Minetest/__/__/src/network/clientpackethandler.o:(Client::handleCommand_Add3DLine(NetworkPacket*))

[fluxionary]

Is there a particular reason or advantage why the 3D lines use a whole new concept rather than re-using and extending existing ActiveObject functionalities

i haven't played around w/ this, but currently you can't attach two entities (e.g. a player and a petz) to a third (a leash), and allow those entities free motion, and dynamically extend/contract the leash as the player and petz move around independently. such a feature would be awesome for implementing e.g. fishing and leashes.

but honestly i have no idea how well this could ever work sanely w/out SSCSM.

also, it'd be awesome if this supported catenary curves and not just straight lines, but that's just a personal wish.

[Andrey2470T]

@appgurueu

[ghost]

also, it'd be awesome if this supported catenary curves and not just straight lines, but that's just a personal wish.

Catenary curves are probably a whole different beast.

[Andrey2470T]

Demonstration of the API usage on the examples of the fishing rod and portal 2 turret:

fishing_rod.mp4

turret.mp4

[srifqi]

Please rebase to recent master branch. The concept itself looks good to me. I have not checked the implementation or tested the code, though.

[Andrey2470T]

@BanishedLord my first post clearly explains the sense of this PR: #13020 (comment)

Also as to why the API is not generalised for supporting any shapes: https://discord.com/channels/369122544273588224/747163566800633906/1062005298228904016

[Andrey2470T]

• axis_angle should simply be called rotation IMO.

I don't think so. Just rotation is vague name. Rotation around what? The first one clarifies that (rotation around axis).

• For flat, it should be possible to specify two textures (one plane with two sides).
• For dihedral, it should be possible to specify two four different textures (two planes with two sides each).
• For cylindric, it should be possible to specify three different textures (one end + the side + the other end).

I'm not sure about whether having various textures on various sides/surfaces is really necessary. Usecases?

• When a new client joins, it doesn't receive the lines that already exist in the world.

I can confirm that. The problem is the server sends updates about lines only to the online clients. If any client is offline, it omits sending packets to that.

• The default values for start_pos and end_pos you specified in lua_api.md don't work. Leaving these fields empty results in an error:

  /mt/bin/../builtin/common/vector.lua:380: attempt to index local 'v' (a nil value)
  stack traceback:
      /mt/bin/../builtin/common/vector.lua:380: in function </mt/bin/../builtin/common/vector.lua:379>
      [C]: in function 'add_3dline'
  

I've fixed that.

• If I create a line like this:

  minetest.add_3dline({
      start_pos = vector.zero(),
      end_pos = vector.new(0, 80, 0),
      start_color = "#ffffff",
      end_color = "#ffffff",
  })

  It has a different, random color each time I restart Minetest. Maybe it tries to load a texture?

Yes, it is strange. I tried #000000 and it works fine.

• Backface culling for "cylindric" lines culls the wrong faces.

  minetest.add_3dline({
      type = "cylindric",
      start_pos = vector.zero(),
      end_pos = vector.new(0, 80, 0),
      start_color = "#ff0000",
      end_color = "#ff0000",
      width = 3,
      texture = "default_snow.png",
      backface_culling = true,
  })

  It looks like this, no matter which side I look from.

Like it is necessary to invert the order of vertices bypassing when they are rendered.

• There are two different default values for width in the code, and there is another different one in lua_api.md. You should refactor the code so that default values are only stored in one place.

Fixed.

• clamp_mode = "clamp" results in lines becoming partially semi-transparent.

  minetest.add_3dline({
      type = "flat",
      start_pos = vector.new(0, 2, 0),
      end_pos = vector.new(0, 80, 0),
      start_color = "#ff0000",
      end_color = "#ff0000",
      width = 3,
      texture = "default_snow.png",
      clamp_mode = "clamp"
  })

  I'm not sure why `GL_CLAMP` and `GL_CLAMP_TO_BORDER` are even exposed. Doesn't `GL_CLAMP_TO_EDGE` suffer for all clamping needs? For `GL_CLAMP_TO_BORDER` to make sense, you need to specify a border color, which you can't do with the current API. So it's useless. And `GL_CLAMP` has apparently even been [removed in the OpenGL 3.2 core profile](https://stackoverflow.com/a/56841375).

I think it worths to remove clamp_mode at all as in the most cases GL_REPEAT is only required.

[grorp]

You've squashed and force-pushed your fixes into the first commit, so I can't actually verify what changes you've made to the code and which bugs you've fixed. Well, I could, but that would waste a lot of time. Please add separate commits when you fix issues pointed out in review comments.

Also as to why the API is not generalised for supporting any shapes: https://discord.com/channels/369122544273588224/747163566800633906/1062005298228904016

Ignoring the actual content for now: Using Discord links to provide essential information is bad because Discord requires you to be registered to see anything. Many community members reject Discord as proprietary. If you link to Discord, you must always provide an alternative.

In this case, you could just have linked to the IRC logs at https://irc.minetest.net instead.

---

EDIT: Another thing: Please resolve all reported issues before requesting a re-review. Most issues reported in the last review aren't resolved yet.

[grorp]

axis_angle should simply be called rotation IMO.

I don't think so. Just rotation is vague name. Rotation around what? The first one clarifies that (rotation around axis).

It's obvious that the rotation will happen around an axis, but around which one? The name axis_angle doesn't make that any clearer. Besides, axis_angle is a confusing name because what you specify there is just an angle, not an axis and an angle as you might assume (see Axis-angle representation).

I'm not sure about whether having various textures on various sides/surfaces is really necessary. Usecases?

For example: Round tree trunks as shown in #13020 (comment). They need a different end texture.

When a new client joins, it doesn't receive the lines that already exist in the world.

I can confirm that. The problem is the server sends updates about lines only to the online clients. If any client is offline, it omits sending packets to that.

That's fixable by maintaining a list of lines on the server and sending all of them when a player joins.

It has a different, random color each time I restart Minetest. Maybe it tries to load a texture?

Yes, it is strange. I tried #000000 and it works fine.

This supports my hypothesis described above.

Backface culling for "cylindric" lines culls the wrong faces.

Like it is necessary to invert the order of vertices bypassing when they are rendered.

So why haven't you fixed this? Or have you?

clamp_mode = "clamp" results in lines becoming partially semi-transparent.
I think it worths to remove clamp_mode at all as in the most cases GL_REPEAT is only required.

The way you've implemented repeating and clamped textures is very inflexible. Modders can't control at what scale a texture is repeated or clamped, they're limited to a scale of one node. Some API design work is needed here.

There are use cases for clamped or stretched textures. For example, a line representing a single arrow seems like a very common use case.

---

You haven't responded to #13020 (review) at all.

[grorp]

There is this very specific call minetest.add_3dline() which is doomed to depreciation because more generally, a primitive drawing API should be preferred to draw basic shapes, of which the lines would be part of it. More thought about this should come into play before merging this.

This is a valid point. I can definitely see use cases for more primitives. Consider e.g. the WorldEdit selection markers that disappear if your area is too big because they're rendered using Lua entities.

While more primitives shouldn't be added to this PR, we could certainly make the API more future-proof by calling it add_primitive and adding a type field to definition tables. What do other coredevs think?

The lines are rendered into a different scene, which means they won't cast the dynamic shadows out of the box, unlike the client map and the entities which are supported in the shadow maps construction. So you need to stack another layer of support and maintenance for the lines' shadow casting. Is it really worth it when you can already draw lines with the entities?

There's another fundamental problem with this PRs approach to rendering: Vertex and index arrays are created for each line for each frame à la immediate mode. They're not only never put into VBOs on the GPU side, they're also not even cached on the CPU side.

Although I'm not a graphics developer, this looks really bad to me. It's definitely possible to do this better, even with Irrlicht, by using SMesh and SMeshBuffer. For an example, see how "upright_sprite" rendering works. The code starts here:

https://github.com/minetest/minetest/blob/3cac17d23e2e0bfe74f1f627a4e206fec981fa4a/src/client/content_cao.cpp#L689

[srifqi]

I agree with renaming functions to make the API more future-proof. I am also removing my +1 until the issues mentioned above have been fixed.

[grorp]

I'm closing this PR and marking it as "adoption needed" since the author hasn't responded to my review comments for more than one month. It can be reopened if the author wants to continue working on it. See also this discussion copied from Discord: https://gist.github.com/appgurueu/e4962b622f0350a3cc90ab1192027c50