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

Brush entities have wrong normals with _phong 1. #101

Open
Xcalibur10 opened this issue Oct 23, 2023 · 4 comments
Open

Brush entities have wrong normals with _phong 1. #101

Xcalibur10 opened this issue Oct 23, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@Xcalibur10
Copy link

I expected brush entities like walls with _phong 1 having really smooth normals which would make the lighting better. Instead, it looks like they are calculated / brush.

Tested a few things: removed the threshold check in GeoGenerator, that kinda confirmed that for me.
I see it supposed to merge vertices. The brush entity I have has skip materials everywhere expect for the surface. I thought it'll fix the vertex merging but no.

So convex meshes are fine. The shading is pretty decent on them. Concave isn't though. What I can't confirm is that the vertices are truly merged. And maybe the normal calculation happens after removing faces that has skip on them? Or I get it wrong and it simply ignores them on mesh building? The mesh itself looks fine, the skipped faces are truly not there. I made some tests in Blender.

Exported the scene from Godot, opened the file in Blender. Bottom is the one from the scene, top one is the same mesh, vertices merged (not sure if gltf exported doesn't export with merged vertices or Blender imports the mesh without merging (even after merging was enabled), normals autosmoothed with 89 degree tolerance (basically the same as default _phong_angle).
image
Replaced the brush with the mesh fixed in Blender
image
wall_phong_bl_norm

The original brush entity with default _phong_angle. Some smoothing presents.
image
wall_phong_tb_norm

@Notclue
Copy link

Notclue commented Nov 10, 2023

Since normal calculation is done in GenerateBrushVertices it means that the normals are only compared for smoothing against other faces in the same brush. Normal smoothing probably needs to be moved to per-entity instead, but unfortunately that's beyond me.

@RhapsodyInGeek
Copy link
Collaborator

Entity mesh generation is performed in SurfaceGatherer.cs. Might be worth starting there?

@Xcalibur10
Copy link
Author

For a workaround, I'll try to write a script that regenerates given meshes after Qodot does it. Waste of processing time, but might work. I realized how the mesh vertices are often not connected, even on same surfaces and actually breaks UV unwrapping too. It works, but the UV2 map looks messy. Causing some ugly shadows here and there, especially with low shadow resolution. Exported my scene into Blender, merged vertices, recalculated normals, Godot now generates nicer UV2 and can improve shading significantly, but it's a really ugly workaround. So I'll try to do that with script at least but I'm a noob with Array Meshes and Surface Tool. So yes, some things should be reordered in Qodot's mesh generation.
Since I never worked with procedural mesh generation, I don't know for example, how to fix vertex indices after merging vertices. Surface tool has normal and tangent generation, I assume if the mesh would be fine (merged vertices), the normal generation would be nice and smooth.
Maybe a post mesh regeneration could be added for entities marked as _phong 1.

Also I'm pretty sure, Qodot's normal generation for phong shaded meshes isn't perfect. Generating normals in Blender vs the code used in Qodot gives different results. Actually in Qodot, checking the normals with the normal view reveals some rough edges. Also visible in normal view, but not that much.

@RhapsodyInGeek
Copy link
Collaborator

RhapsodyInGeek commented Nov 11, 2023

Maybe a post mesh regeneration could be added for entities marked as _phong 1.

If the way Qodot generates meshes is affecting UV2 unwrapping even on non _phong 1 brush entities, then I'd suggest we apply the fix to every brush.

Post mesh regeneration feels more like a band-aid than a fix. My preferred fix would be to just merge the vertices while we're constructing in the first place. I'm not really smart enough to know how best to do that, but we do have access to every vertex in GeoGenerator and SurfaceGatherer.

That said, I wonder what's more performant: checking to see if a vertex point already exists within a certain epsilon and replacing the duplicate vertex with the original (or a pointer to it?), or just post mesh generation replacement at the end of generating a mesh instance?

I also wonder if this affects concave collision shapes, since all collision shapes use SurfaceGatherer to get their points as well?

@DeerTears DeerTears added the bug Something isn't working label Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

4 participants