Implement Bresenham line algorithm in Geometry2D

[groud]

I need it to paint on TileSet, Tilemaps, and it should also be useful for the Gridmap editor.

Note: This implementation is taken from the one in TileMap : https://github.com/godotengine/godot/blob/master/editor/plugins/tile_map_editor_plugin.cpp#L970

[akien-mga]

CC @Xrayez

[Xrayez]

The method is not exposed to scripting in core/bind so I tested this on the C++ level via doctest instead:

TEST_CASE("[Geometry2D] Bresenham line") {
	Vector<Point2i> line = Geometry2D::bresenham_line(Point2i(25, 0), Point2i(49, 49));
	Ref<Image> image;
	image.instance();
	image->create(50, 50, false, Image::FORMAT_RGBA8);
	image->fill(Color(0, 0, 0));
	for (int i = 0; i < line.size(); ++i) {
		image->set_pixelv(line[i], Color(1, 1, 1));
	}
	image->save_png("bresenham.png");
}

Result

So works!

---

Regarding code style, I'd also use Point2/Point2i over Vector2/Vector2i.

And I'd expose the method to scripting. The algorithm can be used for bitmap-based (raycast) collision detection, and I know at least one person who uses such method in Unity. That said, it's not restricted to painting.

[groud]

I don't mind either way, I'll change it to Point2i.

We could eventually expose it I suppose, but performance isn't great.

[groud]

I updated the PR to use Point2i. I also discussed with @reduz about the possibility to expose it, but he thinks, and I agree on this, that the use case stays relatively limited. Considering the performances are not the best, it might be better to keep it internal for now.

If it is needed, I could possibly expose the line drawing algorithm in the TileMap class. It should be enough to cover most use cases.

[Xrayez]

I accept the decision but I'm curious why there's so much opposition to exposing this to scripting? The algorithm is fairly classic so there should be no ambiguity. If performance is concern, then it can be always improved later, and exposing this to scripting may in fact inspire people to make it more efficient. In either case, we'll get either a feature proposal or a bug report out of this. Every new solution creates a problem, and every problem demands solution.

My rationale is that is something is added to core, it should be useful for a lot of use cases, and I interpreted this PR implies this. But to be honest, if this implementation is only useful for tile/grid map editor painting, it should stay there, I'm mostly trying to grasp/adhere to Godot's development principles. Referring to Solutions must be local:

When looking for a solution to a problem, be it implementing a new feature or fixing a bug, sometimes the easiest path is to add data or a new function in the core layers of code.
...
Despite this, this practice is not advised. Generally, the code for a solution should be closer to where the problem originates, even if it involves more code, duplicated [emphasis mine], more complex or is less efficient. More creativity might be needed, but this path is always the advised one.

If this change is an exception from the best practices, I'm curious what makes it so?

[groud]

I accept the decision but I'm curious why there's so much opposition to exposing this to scripting?

I am not sure. But I'd say that the lack of demand for it is likely the first reason.
But that might change over time, we will see if there's a demand for it later on.

But to be honest, if this implementation is only useful for tile/grid map editor painting, it should stay there, I'm mostly trying to grasp/adhere to Godot's development principles.

This would lead the same code to be duplicated at least 4 times (maybe more, as I am trying to split the TileSet editor into smaller modules). As I don't want to make all parts depend on a generic "tiles_helpers.h" files with only one function, I think it's better to have it in Geometry2D.

In any case, we often add things to the core that are needed internally in several places. That does not mean it's always a good idea to expose everything, as some part of it might not be performance optimal, user friendly or even needed.

[Xrayez]

This would lead the same code to be duplicated at least 4 times (maybe more, as I am trying to split the TileSet editor into smaller modules). As I don't want to make all parts depend on a generic "tiles_helpers.h" files with only one function, I think it's better to have it in Geometry2D.

I think, oftentimes, implementation tend to diverge over time due to changing requirements, so what seems to be a code duplication at first may result in quite different result in the future.

According to IRC discussion:

reduz> Groud: yeah but you use it when drawing, not to get a list of points, thats kind of unique to the tile editor, i doubt you want to use this somewhere else
i mean the fact that this is bresehnam is kind of pointless, could be any other less optimized algorithm and it would still be fine

But don't get me wrong, I'm not saying that this shouldn't be implemented in core. But as someone who tends to work on C++ side of things with Godot via modules, I sometimes find myself in a weird/disapponting situation that the C++ has a particular method, but when I switch to GDScript, I cannot find one, which creates more inconsistency and confusion, in my opinion.

I see two possible solutions to this:

• Clearly separate "internal" core methods (despite them being public) from those which are currently exposed to scripting ("truly" public). There are actually several methods which are currently not exposed to scripting, such as make_atlas, pack_rects etc. Separation could be done on the level of documenting this via comments, at the very least.
• Create GeometryUtils2D/GeometryUtils3D class (only on C++ level).

Something similar to VariantInternal class recently implemented in #41715.

I understand that this is outside the scope of this PR, but this is something which would improve code quality (which would also prevent this discussion from happening in the first place). 😛

Also see previous changes such as #15892. I'd like to draw attention to "missing" part there. So, missing bindings could as well be considered as a bug to someone.

But yeah lets see whether users would find this useful at some point, I'm certainly not against these changes in this PR.

[Xrayez]

I also discussed with @reduz about the possibility to expose it, but he thinks, and I agree on this, that the use case stays relatively limited. Considering the performances are not the best, it might be better to keep it internal for now.

For those who are interested, I've implemented Bresenham line in Goost: goostengine/goost#75, which is exposed to scripting.

So, I guess it's fine to keep it internal in Godot indeed, especially if/when the implementation may end up being different depending on the exact use cases.

[Xrayez]

Looks like this method is added as part of another recently merged PR #48535 available in master branch:

godot/core/math/geometry_2d.h

Lines 399 to 435 in a3dda2d

	static Vector<Point2i> bresenham_line(const Point2i &p_start, const Point2i &p_end) {
	Vector<Point2i> points;
	Vector2i delta = (p_end - p_start).abs() * 2;
	Vector2i step = (p_end - p_start).sign();
	Vector2i current = p_start;
	if (delta.x > delta.y) {
	int err = delta.x / 2;
	for (; current.x != p_end.x; current.x += step.x) {
	points.push_back(current);
	err -= delta.y;
	if (err < 0) {
	current.y += step.y;
	err += delta.x;
	}
	}
	} else {
	int err = delta.y / 2;
	for (; current.y != p_end.y; current.y += step.y) {
	points.push_back(current);
	err -= delta.x;
	if (err < 0) {
	current.x += step.x;
	err += delta.y;
	}
	}
	}
	points.push_back(current);
	return points;
	}

Due to this, I think it's safe to close this PR, then.