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

Expose Geometry3D.triangle_box_overlap() to scripting #11467

Open
vandermerschfrancois opened this issue Jan 1, 2025 · 3 comments
Open

Expose Geometry3D.triangle_box_overlap() to scripting #11467

vandermerschfrancois opened this issue Jan 1, 2025 · 3 comments

Comments

@vandermerschfrancois
Copy link

Describe the project you are working on

It's a project that require an extensive use of a triangle-box detection function.

Describe the problem or limitation you are having in your project

Using collider nodes for this would be either extremely inefficient or outright not work at all due to the amount of collider nodes it would require, I tried.

So it took me three days failing to create a functional triangle-box detection function for my project before I decided to check Godot/core to see if there was something similar I could use as a basis, only for me to realize that Geometry3D had the exact function I actually needed all along with no mentions of it's existence and no way to access it.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

It's just exposing triangle_box_overlap that already exists in Geometry3D.
It works, I already tested it in a custom Godot build, but I don't want to compile a new custom Godot build every update just because a helper class does not have the wrapper for the one function I need.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Add that code to godot/doc/classes/Geometry3D.xml :

		<method name="triangle_box_overlap">
			<return type="bool" />
			<param index="0" name="boxcenter" type="Vector3" />
			<param index="1" name="boxhalfsize" type="Vector3" />
			<param index="2" name="a" type="Vector3" />
			<param index="3" name="b" type="Vector3" />
			<param index="4" name="c" type="Vector3" />
			<description>
				Tests if the box ([param boxcenter], [param boxhalfsize]) intersects the triangle [param a], [param b], [param c]. If it does intersect, returns true, if no intersection takes place, returns false.
			</description>
		</method>

Add that code to godot/core/core_bind.cpp :
in Geometry3D

bool Geometry3D::triangle_box_overlap(const Vector3 &boxcenter, const Vector3 boxhalfsize, const Vector3 &p_v0, const Vector3 &p_v1, const Vector3 &p_v2) {
	Vector3 t[3] = { p_v0, p_v1, p_v2};
	return ::Geometry3D::triangle_box_overlap(boxcenter, boxhalfsize, t);
}

in void Geometry3D::_bind_methods()

ClassDB::bind_method(D_METHOD("triangle_box_overlap", "boxcenter", "boxhalfsize", "a", "b", "c"), &Geometry3D::triangle_box_overlap);

And finally add that code to godot/core/core_bind.h :
in class Geometry3D : public Object

bool triangle_box_overlap(const Vector3 &boxcenter, const Vector3 boxhalfsize, const Vector3 &p_v0, const Vector3 &p_v1, const Vector3 &p_v2);

If this enhancement will not be used often, can it be worked around with a few lines of script?

No.

Is there a reason why this should be core and not an add-on in the asset library?

It is in core, and as a matter of fact i think it's pretty stupid that a helper class has it's functions inconsistently wrapped, and the others functions not wrapped in Geometry3D should also be exposed.

@RedMser
Copy link

RedMser commented Jan 2, 2025

Makes sense, but since you already need to make a wrapper due to the points a b c, maybe the public-facing API could be improved further to use the AABB class instead of boxcenter and boxhalfsize.

@Calinou Calinou changed the title Expose triangle_box_overlap in Geometry3D Expose Geometry3D.triangle_box_overlap() to scripting Jan 2, 2025
@vandermerschfrancois
Copy link
Author

Makes sense, but since you already need to make a wrapper due to the points a b c, maybe the public-facing API could be improved further to use the AABB class instead of boxcenter and boxhalfsize.

I'm not sure it would be a good idea, the point of Geometry3D is to be an helper class that give access to advanced geometry functions using basic data.
Having the wrapper directly use AABB would just end up restricting the usability of the function as you would be forced to get or build an AABB for every box you want to check, which would end up being almost as wasteful as using colliders to get the same result if the boxes are meant to be virtual, for example in the context of a voxel grid.

I think keeping it simple is better, if you want to use AABB you can create a wrapper in your project (which i did for my testing of the function), it's as simple as getting an AABB, getting the center using AABB.get_center(), and getting the halfsize by getting the AABB.size /2.

Or just use it in the same way as the engine scripts use it for this exact purpose, like here :

Vector3 qsize = aabb.size * 0.5; //quarter size, for fast aabb test

if (!Geometry3D::triangle_box_overlap(aabb.position + qsize, qsize, p_points)) {
	//does not fit in child, go on
	continue;
}

or here :

//test against original bounds
if (!Geometry3D::triangle_box_overlap(aabb.get_center(), aabb.size * 0.5, face.vertex)) {
	continue;
}

@RedMser
Copy link

RedMser commented Jan 3, 2025

forced to get or build an AABB for every box you want to check, which would end up being almost as wasteful as using colliders to get the same result

An AABB variant can just be constructed like AABB(pos, size), it only stores two Vector3s, it doesn't tie to the scene tree, so the complexity and cost is basically zero (about as much cost as constructing Vector3 instead of 3 floats).

Though I do now see the cost of having to compute the center and halfsize, given an AABB. I looked into the algorithm code and it doesn't derive any user friendly values from these which would better serve as inputs.
While the conversion logic is "just" size/2 and pos+halfsize, I agree that this would just add unnecessary steps to force computing every time.


In that case I recommend as a change: rename boxhalfsize to box_extents, as this is what other APIs in Godot call this concept. Ses the docs of build_box_planes which nicely explain this parameter's meaning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants