-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
[3.x] Add MergeGroup node to simplify merging Meshes at runtime #61568
Conversation
If you add some sort of a display in-editor (AABB box around the merge group? view mode like portals?), this would effectively obviate the editor UI pr... achieving the same result while being more useful for scripting/procedural use cases |
Yes this approach has a lot of advantages. I might have to look into whether the speed can be improved though to prevent delays on loading levels (EDIT: yes some small optimization tests last night to the merging code give more than 2-3x speed increase), or whether some merging can be done on export. |
@lawnjelly what is the main benefit of this? |
less draw calls |
See #57661 for example project that shows 24x speedup in frame rate due to reduced drawcalls. OpenGLES in particular can be very bottlenecked by drawcalls / statechanges. And the engine itself operates more efficiently with lower numbers of nodes (although culling can be less accurate). |
lawnjelly what is your strategy for getting this into godot 4? I wanted to try implementing texture merging. This idea of cached merging resolved my concern with baking. So I want to try to do this for 4. |
These two PRs are very much proof of concepts / probably to go with a proposal, and one of those things that it is as quick to write a draft PR as a proposal, so it made sense to do them together with some example working versions (these wouldn't be till 3.6 anyway so there is plenty of time). Mesh merging is more immediately useful in 3.x than 4.x, because OpenGL is often bottlenecked by drawcalls, whereas with Vulkan the benefits may be less clearcut - although with the GLES3 renderer in 4.0, it still could be very useful. It would probably be best discussed with @reduz first for 4.x in terms of proposal, whether it is worth doing, and which approach should be taken, before spending a long time on it (or texture merging, which is more involved). I also suspect the merging may be able to be done in a synergistic fashion with the HLOD. I'm not yet familiar with the HLOD in 4.0. |
9e8bf76
to
2aa1bcf
Compare
b8bd9d8
to
7434a1a
Compare
Is there a forward port of this to 4.0? I couldn't find it. |
See discussion in godotengine/godot-proposals#4630 (comment) . TLDR - This is more relevant for 3.x because of the cost of drawcalls / state changes in GLES. As I suspected reduz is more sceptical about adding this in 4.x because in vulkan (at least) these are much cheaper. We can experiment with it as an extension though (at a later date). |
c914982
to
9a18f6f
Compare
I now have the This is kind of related to godotengine/godot-proposals#182 , although I think they are more concerned with the editor. Another additional option is to make this baking available in #61564 , and have a destructive bake operation available in the editor, which may be closer to that proposal. EDIT: You can now optionally bake everything in the editor with a button, so pre-baking CSGs is possible. It bakes to a user specified tscn file, so less of an issue with this being destructive and the possibility of losing work. You can also bake |
Either approach is fine imho. I just wish this was merged in time for beta so that I can take it for a runtime spin and see how it works (after all, I have yet to see CSG being a editor runtime hog as opposed to e.g. GI) <3 Those approaches can be discussed later (hopefully with some more people than just me chiming in :P ) EDIT: And I keep forgetting this PR is open against 3.x not master |
Tested locally (rebased against
I'll have to look into doing performance testing, especially on Android and integrated graphics. |
f9735fc
to
25515af
Compare
Mentions the number considered for merging. The number of resulting meshes isn't straight forward as it is subject to further splitting and joining. Additionally when I tried to count the meshes at the end there is a race condition due to scene processing on another thread, and it seemed a lot of hassle to solve for such an info tip.
Not sure yet whether this can be done in efficient manner. Will see.
Yes, to remain non-destructive. I had a think about this last night. This proposed change assumes a certain workflow which may not be the case. So it might be good to leave as a follow up PR, as it is essentially a quality of life change that can be considered on its own merit.
Again this assumes workflow and should probably be in a follow up PR. |
Something that I've thought about and there is no easy way around in 3.x, is that I'm acutely aware there can be too many options in the inspector. We have no good way of hiding options behind an advanced toggle. Some of the options like It seems like if going for the trouble of duplicating things to put them in the baking dialog, it would probably be as much effort to write an advanced toggle for the inspector. Maybe I'm overthinking this, and given 3.x is end of life, we should just accept these limitations. But maybe I'll have a go tomorrow at moving them to the baking dialog, it could be a good compromise. UPDATE: The trade off here is that advanced users would need to write a script to set the parameters for the conversion at runtime. UPDATE: |
0e109ec
to
8e62f94
Compare
3a2121d
to
7ac6f02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed I reviewed the code to look for red flags. But have not done a fulsome review. At this point, I think it is unlikely that I can do a proper review before 3.6, so we agreed to go ahead as long as there are no obvious red flags.
Accordingly, I think We should merge this PR (pending the small comments I have left here)
And for future reference, the plan here is to test out this node in 3.6 and consider forward porting to 4.x if 1) users like the interface and find it easy to use and 2) there is a justifiable performance increase in real projects.
Done. I also added one tiny change I forgot, a tooltip for the bake button, "Bake MergeGroup to Scene.". |
Thanks! |
I am here from the 3.6 release update, and have a question on this
Does Gridmap (in 4.x) work the same way in the renderer, as this MergeGroup feature?
I think this is the ideal feature for 3D destructible enviroments by explosive weapons (e.g. rainbow six siege, battlefield games, battlebit, world of tanks) where you can partly destroy walls and buildings, which I cannot think implementing in 4.x without this, performance-wise. |
Have to leave this to the 4.x guys, but afaik 4.x can do some degree of auto-instancing when duplicate objects are detected. This is not quite the same thing, as it relies on objects being duplicates, but could alleviate performance problems in some situations. On the other hand with merging, you could even potentially procedurally shatter objects, then merge the results so they are rendered in a single draw call. On the flip side, auto-instancing is afaik automatic, whereas merging requires a certain degree of thinking ahead for these situations. You may be able to e.g. place the shattered area under a |
Perfect, so Gridmaps are very efficient, thank you for the answer, and ofc for this new MergeGroup feature 🎉
🏆 |
MergeGroup node will automatically merge suitable child / grandchild meshes when added to the scene.
Only active when running outside the editor.
Alternative approach to #61564 , using the functionality from #57661 .
Current Features
SurfaceTool
for merging vertex data. This reduces code duplication.SurfaceTool
has been improved to useLocalVector
rather than linked lists (this has already been done in master). This is necessary to get the speed required. It is slightly slower than the bespoke code that was previously in this PR by a factor of approx 4:3, but the benefits to reducing code duplication outweigh this, especially in terms of tangents.merging_allowed
flag in the Inspector forSpatial
, which is managed as an inherited propertymerging_mode
.(This replaces the
allow_merging
flag inCullInstance
, as it is more versatile, and portals now respects this new flag. The legacyallow_merging
flag is still respected, but deprecated. )shadows_only
flag.CSG
s toMeshInstance
s, which can be further merged.GridMap
s toMeshInstance
s, which can be further merged.MeshInstance
s with multiple surfaces into oneMeshInstance
per surface prior to main merging stepScene cleanup
After deleting meshes that were sources for merging, there is now a certain amount of cleanup that goes on, removing these source nodes, and moving their children to appropriate places (usually to be children of the parent of the removed node).
Shadow Proxies
This node can also be used for automatically creating merged "shadow proxies". I have added further backend to the merging to allow merging shadow meshes even that do not share the same materials, as long as they are opaque. If you tick the
shadow_proxy
checkbox, a single shadow proxy will be created, and set to cast shadows. The source meshes will be set to no longer cast shadows.In this way rendering the shadowmaps can be done with far fewer drawcalls (at the cost of decreased culling accuracy). Additionally normals, uvs etc are stripped from the shadow proxy mesh.
Baking Merged Scenes
The baking dialog exposes the full set of merging options. These are normally hidden in the merging node, but can be set at runtime by setting the
auto_merge
to false, and manually setting the parameters in script prior to callingmerge_meshes()
.These were previously exposed as properties on the merging node, but may have been confusing for new users. Hiding the properties still allows the functionality for advanced users. If there is demand we can re-add selected properties at a later date.
Example - Pirate Ship
If our pirate ship has 1000 objects using 10 different materials (woods, metals) and the ship root node is a
MergeGroup
, it can be converted at runtime to 10MeshInstance
s (or 1MeshInstance
with 10 materials), and 1 shadow proxyMeshInstance
. Shadow casting can be turned off for the mainMeshInstance
s, and the shadow proxy can cast shadows only.For a typical example with a directional light (4 splits) and 2 other spotlights, we might go from:
This is assuming the ship is opaque and all the objects in the ship are static relative to each other. If you wanted e.g. a moving sail / rigging this would be outside this
MergeGroup
.Example - TruckTown demo
TruckTown
node from aSpatial
to aMeshGroup
.auto_merge
to turn merging on and off, examine debug monitor:The shader changes actually went up.. not sure what that was about 😁 (ah it seems something to do with the 4 split directional light, maybe it's not culling as efficiently after merging. This should be helped by the shadow proxies though which I'm currently adding.). But the drawcalls etc are all a lot lower. The vertex count is slightly higher because the culling will be less accurate.
UPDATE: With the new shadow proxy mode, the drawcalls is reduced again for trucktown to between 30-40.
Merging techniques
Although the original merging code was principally designed for merging identical (or very similar) objects, that shared identical materials, the new
MergeGroup
can now split sourceMeshInstances
into several intermediateMeshInstances
, one for each surface. This makes it more likely a match will be found for merging.For example if object A contains wood and metal, and object B contains metal and plastic, with the old system they could not be merged. But if we split them into 3 objects based on material, then we can merge any number of these objects into just 3
MeshInstances
.MeshInstances
with non-matching materials to form anUberMesh
, that contains many surfaces, each with different materials. This can be culled as one, and forms a single node. The downside is that it culls as one, so culling accuracy may be reduced.Notes
MergeNode
toMergeGroup
, but am happy to go with what we all think is most easy to understand.MeshInstances
to be considered mergeable - theGeometryInstance
data,VisualInstance
andCullInstance
.It may be worth adding to the verbose logging the reason why a particular pair of meshes can't be merged, and / or having a way to loosen the requirements for merging, as it could be annoying not knowing why meshes haven't merged if they have e.g. a slightly different
cull_margin
or something like that. But these are things that could be changed based on feedback in a later PR.