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

MeshDataTool.get_face_vertex() has a unclear description. #7852

Closed
church-basement opened this issue Aug 28, 2023 · 2 comments · Fixed by godotengine/godot#81088
Closed

MeshDataTool.get_face_vertex() has a unclear description. #7852

church-basement opened this issue Aug 28, 2023 · 2 comments · Fixed by godotengine/godot#81088
Labels
area:class reference Issues and PRs about the class reference, which should be addressed on the Godot engine repository enhancement

Comments

@church-basement
Copy link

Your Godot version:
Godot v4.0.2

Issue description:
The function description for MeshDataTool.get_face_vertex() is unclear as to the data type that is returned.

Returns the specified vertex of the given face.
Vertex argument must be either 0, 1, or 2 because faces contain three vertices.

This is a bit confusing since the function returns the vertex index.
May I suggest something like this?

Returns the specified vertex index of the given face.
Vertex argument must be either 0, 1, or 2 because faces contain three vertices.

As a side note, it would make more sense if this function returned a vertex (Vector3) instead of a vertex index (Int).
Maybe that's just me.

URL to the documentation page (if already existing):
https://docs.godotengine.org/en/4.0/classes/class_meshdatatool.html

@clayjohn
Copy link
Member

I think your suggestion is good. A code example would work well here too:

var index = mesh_data_tool.get_face_vertex(0, 1) # Gets the index of the second vertex of the first face.
var position = mesh_data_tool.get_vertex(index)
var normal = mesh_data_tool.get_vertex_normal(index)

As a side note, it would make more sense if this function returned a vertex (Vector3) instead of a vertex index (Int).
Maybe that's just me.

I don't think so. As you need the index to access all other vertex attributes as well (normal, uv, etc.)

@mateuseap
Copy link
Contributor

Done! I've added the new description suggested by @kravohi and the code snippet suggested by @clayjohn, now I think the method reference is quite good.

@Piralein Piralein added the area:class reference Issues and PRs about the class reference, which should be addressed on the Godot engine repository label Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:class reference Issues and PRs about the class reference, which should be addressed on the Godot engine repository enhancement
Projects
None yet
5 participants