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

Bind mesh merging functionality in MeshInstance #57661

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Feb 5, 2022

The portal system introduced basic mesh merging functionality, this has wide ranging uses outside of the portal system.
For this reason, this PR binds the mesh merging functionality.
It also slightly modifies the calling from RoomManager to use a Vector of Node *, in order to allow binding of the function.

Introduction

Mesh merging is a very useful tool in the performance toolbox. Many games, especially outdoor games are bottlenecked by the number of MeshInstances, both in terms of draw calls in the renderer (especially with OpenGL), but also in terms of housekeeping scene tree side.
Merging meshes, either ahead of time (pre-baking and saving) or at runtime (usually at level startup) can offer significant performance improvements, particularly for things like vegetation and procedural modular block levels (such as minecraft type maps).
This is especially relevant when hardware instancing is not available, such as in GLES2, or where hardware instancing cannot be used, such as when a variety of different meshes must be drawn at once.

Note however that there is a trade off - merged meshes will be culled as a block, so depending on how you apply it you can lose culling accuracy. This will still be a net win in many situations though. And, unlike instancing, you also lose the ability to move the merged objects relative to each other, they can only be moved as a group.

Local or Global space

The merged vertices can either be defined in global (world) space, or in the local space of the destination MeshInstance (provided it is in the SceneTree, which is necessary in order to get its global_transform).

In a static scene, global space may make sense, but when merging sub-parts of a movable object (say a spaceship), you may want the vertices specified relative to the object, so that when you move it the parts move in unison.

Local space is default. Note that local space will flag a warning if you merge before adding the MeshInstance to the SceneTree (because the local space is undefined), and will revert to global space.

Notes

  • The mesh merging functionality is already available in Rooms & Portals, so it would seem logical to make it available for general use, as there are many other use cases outside the portal system.
  • The system initially offers only very simplistic merging (in particularly materials must match) but it should be suitable for many use cases.
  • Suggestions welcome for better names for the functions that are exposed, or improvements to the API (especially for future considerations).
  • It may be desirable at some stage to add something to the UI to give this a more user-orientated experience, but this is not necessary to get the ball rolling for people who want to use this.
  • Merging from source meshes that are not in the SceneTree will work (and just use an identity transform for the source mesh), but it will flag a benign error from calling get_global_transform(). I'm not quite sure whether it would be better to check is_inside_tree() and silently allow this, we can always change this based on feedback though.

Example script:

	print($MeshInstance.is_mergeable_with($MeshInstance2))
	
	var arr = []
	arr.push_back($MeshInstance)
	arr.push_back($MeshInstance2)
	
	$Merged.merge_meshes(arr)

Example project

20,000 boxes, either drawn separately or merged:
MergingTest.zip

Separate 28 fps
Merged 664 fps (24 x faster)

Bugsquad edit: Related (but not closing) godotengine/godot-proposals#901.

The portal system introduced basic mesh merging functionality, this has wide ranging uses outside of the portal system.
For this reason, this PR binds the mesh merging functionality.
It also slightly modifies the calling from RoomManager to use a Vector of Node *, in order to allow binding of the function.
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in PR review meeting today.

Exposing the existing feature for 3.x seems fine, so this can be merged.

For 4.0, we'd like to have this properly integrated in the importer so that mesh merging can be done at import, also handling LOD, etc. properly.

@akien-mga akien-mga merged commit 6a524b2 into godotengine:3.x Mar 17, 2022
@akien-mga
Copy link
Member

Thanks!

@Zireael07
Copy link
Contributor

I noticed this PR is for 3.x branch - is there an equivalent for 4.0?

@Calinou
Copy link
Member

Calinou commented Mar 17, 2022

I noticed this PR is for 3.x branch - is there an equivalent for 4.0?

See the above comment:

For 4.0, we'd like to have this properly integrated in the importer so that mesh merging can be done at import, also handling LOD, etc. properly.

This will require an entirely separate implementation – also, the portals and rooms system isn't ported to 4.0 yet. Mesh merging relies on the portals and rooms system to an extent – at least internally.

@lawnjelly lawnjelly deleted the bind_mesh_merging branch March 21, 2022 13:22
@TokisanGames
Copy link
Contributor

@akien-mga This seems like a pretty big deal that our team just discovered by pouring over PRs. We'll be implementing it soon and expect it will give us a huge performance boost on our development level. We were previously merging meshes manually in blender and it was quite time consuming. We looked at your latest news article and saw it's listed in the additions list, but it seems like this is a very important feature that should be highlighted in your news release.

Thanks to everyone who made it possible. Looking forward to mesh merging in GD4. It's definitely desireable to have meshes able to be merged on import or at runtime. Runtime allows us to construct a house or village from components and keep them separate for development.

@lawnjelly
Copy link
Member Author

lawnjelly commented May 31, 2022

Mesh merging is indeed hugely important, especially on GLES as drawcalls will often limit performance. Ideally it would be nice to expose this in an easier to use way (such as the UI, or automatic), but at least now it is available for power users.

This is something we discussed recently on rocketchat, the potential for static flags (or groups) for meshes or similar to allow them to be merged after loading a level at runtime, both in terms of camera rendering (where materials must match) and in terms of shadow proxy meshes (where there is no requirement for matching materials, providing they are opaque and not moving).

One even simpler option is to add a menu item whereby you can select a bunch of MeshInstances, and then click merge in the Editor UI and it would create a new merged MeshInstance (or output an error message if they can't be merged due to mismatching materials). And perhaps a similar one for creating shadow proxies that doesn't care about material, and discards normals, UVs and unnecessary vertex data. It might also be nice to have a similar function for creating decimated meshes for LOD (see #53778). These could be used in conjunction because you might e.g. merge a group of houses, then decimate them as a group.

@TokisanGames
Copy link
Contributor

With "matching materials", do they have to be an identical 1:1 match? The documentation isn't clear on the specifics.

If a wall has 2 materials, rock and wood, that should merge with a beam that only has wood. Really, even if you merge a single material wood beam and a rock, it should just create two material slots in the merged mesh. If you merge a set of houses, it should align all the glass, wood, thatch materials, and even though your window object is just a plane with one material, create additional slots for unique materials.

@lawnjelly
Copy link
Member Author

lawnjelly commented May 31, 2022

It is very basic at the moment, primarily intended for merging bunch of similar (or identical) objects like trees, so the material matching requirements are very high. Merging can be made quite a bit more versatile though, by creating multiple materials, or even creating atlases. @fire has done quite a bit in this area I believe. PRs to improve the merging are welcome.

Bear in mind that a single MeshInstance using e.g. 5 materials isn't that much cheaper that 5 MeshInstances. It can be a better approach to first split them by material, then merge into separate MeshInstances each with a separate material. That way they can be culled independently. My splerger addon https://github.com/lawnjelly/godot-splerger contains functionality for splitting by material.

@TokisanGames
Copy link
Contributor

TokisanGames commented May 31, 2022

Bear in mind that a single MeshInstance using e.g. 5 materials isn't that much cheaper that 5 MeshInstances.

I'd expect that having a house like this w/ 6-7 materials, 786 objects, 1859 draw calls should get reduced down to around 6-7 draw calls based on the number of materials. And when we have 30 houses in our village, that magnifies the savings.

image

However I tried some merging of simple objects via code and could not get it to work. The engine said they were mergable, and said the merge was successful, yet the resulting mesh is the same as the appended mesh, the source mesh is not included. Also the materials moved. They were attached to the mesh, after the merge they were put on the material override slots, which is not where they should be.

Begin:
image

End:
image

tool
export(Mesh) var mesha: Mesh
export(Mesh) var meshb: Mesh
export var start_merge: bool = false setget _start_merge 

func _start_merge(value: bool) -> void:
	if value == true:
		var mi1 := MeshInstance.new()
		var mi2 := MeshInstance.new()
		
		mi1.mesh = mesha
		add_child(mi1)
		mi1.name = "MI1"
		mi1.set_owner(get_tree().get_edited_scene_root())

		mi2.mesh = meshb
		add_child(mi2)
		mi2.name = "MI2"
		mi2.set_owner(get_tree().get_edited_scene_root())

		if mi1.is_mergeable_with(mi2):
			print("%s can be merged with %s" % [mi1.name, mi2.name])
			if mi1.merge_meshes([mi2], true, false):
				print("Merge successful")
		else:
			print("%s canot be merged with %s" % [mi1.name, mi2.name])

@lawnjelly
Copy link
Member Author

I'd expect that having a house like this w/ 6-7 materials, 786 objects, 1859 draw calls should get reduced down to around 6-7 draw calls based on the number of materials. And when we have 30 houses in our village, that magnifies the savings.

Oh yup sure. You would have to split by material first (using the addon), or improve the merging code. As I say it is currently bare minimum, and designed mainly with duplicate instances in mind (in order politically to get it merged, it is easier when things are simpler, rather than write something complex and not have it merged at all).

Also the materials moved. They were attached to the mesh, after the merge they were put on the material override slots, which is not where they should be.

Does the model look different? The merging only attempts to make the result look the same, it does not warrant that the technical aspects will bear any relation to the input meshes (especially to make the code as simple as possible). It is possible that the materials could be set in a better way, I'm no expert in how materials are specified.

The engine said they were mergable, and said the merge was successful, yet the resulting mesh is the same as the appended mesh, the source mesh is not included.

Please open an issue with MRP and I can investigate. 👍
Bear in mind that the mesh merging is not yet highly tested, it is quite hidden until we have it available from the UI, so bugs should be expected as there are a lot of possible combinations of input meshes etc.

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

Successfully merging this pull request may close these issues.

5 participants