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

Empty mesh causes SurfaceTools::generate_tangents to fail an assertion which doesn't make sense. #92395

Closed
pl7y opened this issue May 26, 2024 · 6 comments · Fixed by #93326
Closed

Comments

@pl7y
Copy link

pl7y commented May 26, 2024

Tested versions

  • Reproducible in 4.2 stable

System information

OSX Sonoma 14.4.1 (23E224)

Issue description

The following error is shown in console:

E 0:00:01:0467   test.gd:22 @ _ready(): Condition "!(format & Mesh::ARRAY_FORMAT_TEX_UV)" is true.
  <C++ Source>   scene/resources/surface_tool.cpp:1151 @ generate_tangents()
  <Stack Trace>  test.gd:22 @ _ready()

If empty meshes are allowed, assertion should not be triggered.
If empty meshes are not allowed, assertion should be checked only if there is some mesh set.

Steps to reproduce

Create a node and add this code to the _ready() function:

func _ready():
  var st = SurfaceTool.new()
  st.begin(Mesh.PRIMITIVE_TRIANGLES)
  st.generate_normals()
  st.generate_tangents()
  var mesh = st.commit()  

Minimal reproduction project (MRP)

See "steps to reproduce".

@AThousandShips
Copy link
Member

AThousandShips commented May 26, 2024

This fails because you need to have UV to generate tangents as they are related to textures

The error message should be made clearer, but this retirement is clearly documented:

Generates a tangent vector for each vertex. Requires that each vertex have UVs and normals set already (see generate_normals).

@clayjohn
Copy link
Member

I agree, the error should be expanded to include a clear message saying that UVs are required to generate tangents.

@Maski0
Copy link
Contributor

Maski0 commented Jun 18, 2024

(This is my First time Contributing) @clayjohn I would like take up this Issue.
Currently for The Error ERR_FAIL_COND(!(format & Mesh::ARRAY_FORMAT_TEX_UV)); Macro is being
I will be simply using ERR_FAIL_COND_MSG Macro with message saying "UVs are required to generate tangents"
is the above approch good, or is there any better approch. And also if we change the above do i have to change anything else(like documentation)

@clayjohn
Copy link
Member

(This is my First time Contributing) @clayjohn I would like take up this Issue. Currently for The Error ERR_FAIL_COND(!(format & Mesh::ARRAY_FORMAT_TEX_UV)); Macro is being I will be simply using ERR_FAIL_COND_MSG Macro with message saying "UVs are required to generate tangents" is the above approch good, or is there any better approch. And also if we change the above do i have to change anything else(like documentation)

Your suggested approach is correct and you don't need to change anything else :)

@Maski0
Copy link
Contributor

Maski0 commented Jun 18, 2024

Your suggested approach is correct and you don't need to change anything else :)

Ok thank you sir, also a rookie question I am guessing I will have to create a pr for the master itself or for 4.2 branch as the tested version mentioned above.

@clayjohn
Copy link
Member

You only need to make a PR for the master branch. We can "cherrypick" the change to the 4.2 branch automatically.

akien-mga added a commit that referenced this issue Jun 20, 2024
Improve `SurfaceTool::generate_tangents` UV error message
@akien-mga akien-mga added this to the 4.3 milestone Jun 20, 2024
fire pushed a commit to V-Sekai/godot that referenced this issue Jul 2, 2024
MewPurPur pushed a commit to MewPurPur/godot that referenced this issue Jul 11, 2024
sorascode pushed a commit to sorascode/godot-soras-version that referenced this issue Jul 22, 2024
Luis-Wong pushed a commit to Luis-Wong/godot that referenced this issue Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants