Make SurfaceTool.generate_normals() behave consistently with smoothing groups

[Klowner]

I'm not entirely sure how smooth groups is supposed to work within SurfaceTool since it seems to be undocumented, but I think this relatively small change makes the behavior easier to reason about and produce results in a consistent manner.

This change addresses a couple problems(?) with generate_normals(). The first issue is that vertices are binned using a hash map which takes into account the entire vertex (including uvs, colors, bones, etc.). To address this issue, this PR introduces a SmoothGroupVertex struct and accompanying SmoothGroupVertexHasher which only takes vertex position and smooth group into account. This means that vertices of adjacent faces sharing the same smooth group will contribute to and receive smoothed normals regardless of other vertex attributes.

The other potential issue that some other tickets (see below) bring up is that for a user to generate normals for a mesh with flat faces it would be necessary to use different smooth groups for each face (or at least ensuring that no adjacent vertices share smooth groups) which seems to me like an unnecessary burden for the user. This PR also includes changes which imply that surface group 0 is meant to be flat shaded. If a user wishes to create a smooth shaded mesh, a single call to surface_tool.set_smooth_group(1) would mean the entire mesh will be smooth. This behavior makes sense to me, but I defer to the group 😁 .

Here's a screenshot of the output of the attached example project (adapted from a related issue below):

Example:
SurfaceToolSmoothShading4.zip

Related issues:

• SurfaceTool normals can't be smooth when UVs are added #6146 - (2.x and 3.x) describes an issue where smoothing is not performed when vertices of adjacent faces don't share UV coordinates (binning issue)
• SurfaceTool.generate_ normals cannot be used normally #65076 - (4.0) - claims 3.x works fine, but the example consists of flat sided cubes which masks the issue, while the problem is clearly visible in the 4.x screenshot (also a binning issue)
• SurfaceTool.add_smooth_group() has no effect #65104 - (3.5) - identifies part of the vertex binning issue, but 3.x's add_smooth_group() has a slightly unrelated bug in addition to the binning.

Although this PR is not applicable to 3.x, I think similar behavior can be implemented with the old add_smooth_group(bool) method.

Hopefully this is a welcome contribution, any feedback is greatly appreciated!

Bugsquad edit:

• Fixes SurfaceTool normals can't be smooth when UVs are added #6146
• Fixes SurfaceTool.generate_ normals cannot be used normally #65076
• Fixes SurfaceTool.add_smooth_group() has no effect #65104
• Fixes Smooth geometry no longer possible with SurfaceTool #66765

[kleonc]

Breaks compat as it changes the default behavior (although it could be treated as a bugfix).

Can't tell if that's the best / preferred approach but the justification makes sense. The changes should probably be fine as long as the new behavior is documented. So yeah, please update the docs (SurfaceTool.xml) according to the changes made in this PR as well.

cc @clayjohn? (kinda related to rendering)

[Klowner]

Added docs!

[clayjohn]

See also: #65113

I don't love using 0 as the setting to disable smooth groups, I would much prefer -1, although using -1 would require us to use an int64_t instead of a uint32_t for smooth group.

I think adding SmoothGroupVertex so that vertices with different UV coordinates (for example) can still be smoothed makes sense. But will need @reduz's input as he is still the owner of this code

[pseidemann]

hi,
nice change! I just ran into this as well.
does this change also fix a potential bug that UVs are considered when comparing vertices when searching for duplicates e.g. in index()?
right now I think there is a related bug that once I set different UVs for vertices, not only set_smooth_group() does not work properly but also index() and I actually think also commit() will generate more vertices than needed because of this.

might be worth fixing the overall definition of when two vertices are considered identical/redundant?

[pseidemann]

is there any way to smoothen the normals after committing in go 3.5 maybe with MeshDataTool?

[pseidemann]

after some fiddling I was able to write a function which computes smooth normals for a given mesh (should work with any godot version):

func smooth_normals(input_mesh, output_mesh):
	var mdt = MeshDataTool.new()

	# read input mesh
	mdt.create_from_surface(input_mesh, 0)

	# group vertices by position
	var grouped = {}
	for i in range(mdt.get_vertex_count()):
		var pos = mdt.get_vertex(i)
		if !grouped.has(pos):
			grouped[pos] = []
		grouped[pos].push_back(i)

	# process every distinct position only once
	for pos in grouped:
		var vertex_ids = grouped[pos]
		var adjacent_faces = []

		# iterate all vertices with the same position
		for i in vertex_ids:
			var faces = mdt.get_vertex_faces(i)
			# put all faces of all vertices with the same position in one list
			adjacent_faces.append_array(faces)

		# get sum of all face normals of all adjacent faces of current position
		var normal = Vector3.ZERO
		for i in adjacent_faces:
			normal += mdt.get_face_normal(i)
		normal = normal.normalized()

		# set computed normal for all vertices with same position
		for i in vertex_ids:
			mdt.set_vertex_normal(i, normal)

	# write to output mesh
	mdt.commit_to_surface(output_mesh)

[Klowner]

@pseidemann yep, this change fixes the issue where smoothing isn't performed if UVs are incongruous.

Considering how indexed rendering works, index() creating additional vertices when setting UVs isn't so much a bug as it would be expected behavior, no?

I'm also happy to remove the 0 indicating flat shading from this PR.

[pseidemann]

@Klowner

Considering how indexed rendering works, index() creating additional vertices when setting UVs isn't so much a bug as it would be expected behavior, no?

true

[clayjohn]

I'm also happy to remove the 0 indicating flat shading from this PR.

Without that change this PR is a bugfix and we could go ahead and merge it without @reduz's review.

That being said, I agree that needing to specify a unique smooth group per vertex to get flat shaded normals is awkward at best. So it may be worthwhile to wait for his input so we can improve the behaviour while we are at it

[clayjohn]

We discussed this on the rendering Rocketchat and agreed that -1 would be a better value for specifying flat normals. I rebased this PR and made the change in this commit clayjohn@4f26b57

@Klowner Will you have a chance to incorporate this into your PR sometime this week? If not, I can make a PR with your changes and list you as the co-author.

[Klowner]

Oh, excellent, yes I'm more than happy to incorporate that. Thank you!

…

On Tue, Jan 24, 2023, 2:23 PM Clay John ***@***.***> wrote: We discussed this on the rendering Rocketchat and agreed that -1 would be a better value for specifying flat normals. I rebased this PR and made the change in this commit ***@***.*** <clayjohn@4f26b57> @Klowner <https://github.com/Klowner> Will you have a chance to incorporate this into your PR sometime this week? If not, I can make a PR with your changes and list you as the co-author. — Reply to this email directly, view it on GitHub <#68034 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACJHTNFDEGMA3M7AJH7FTWUA24JANCNFSM6AAAAAARR67H7I> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Klowner]

Added your change, @clayjohn. Also rebased atop master and incorporated the for (v: vectors) iterator syntax (which I assume is the new preferred style). 🕺

[clayjohn]

Thank you for updating this so quickly! Looks good to me

[clayjohn]

The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

[Klowner]

Ah, thank you, flat as a pancake 👍

[akien-mga]

Thanks!

[akien-mga]

Related issues:

• SurfaceTool normals can't be smooth when UVs are added #6146 - (2.x and 3.x) describes an issue where smoothing is not performed when vertices of adjacent faces don't share UV coordinates (binning issue)
• SurfaceTool.generate_ normals cannot be used normally #65076 - (4.0) - claims 3.x works fine, but the example consists of flat sided cubes which masks the issue, while the problem is clearly visible in the 4.x screenshot (also a binning issue)
• SurfaceTool.add_smooth_group() has no effect #65104 - (3.5) - identifies part of the vertex binning issue, but 3.x's add_smooth_group() has a slightly unrelated bug in addition to the binning.

Are those 3 fixed by this PR?

[Klowner]

Yep! fixes #6146, fixes #65076, and fixes #65104.

[Klowner]

Fixes #66765 as well, since it should now be possible to disable smoothing.