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

Make MeshLibrary export do recursive depth-search for MeshInstance3D nodes #87923

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented Feb 4, 2024

Makes MeshLibrary export do recursive depth-search for MeshInstance3D nodes.

Fixes #85085
Fixes #81850
Likely other issues as it has been a long-standing issue with the MeshLibrary exporter.

Would supersede #82792 and #87651 that increases the number of searched child tiers to a hard-coded higher number.

The current MeshLibrary export has the limitation of only searching 2 node child tiers.

In the wild there are often convoluted files (e.g. GLTF import) with far more nested child nodes so those mesh items would not be detected and exported.

This pr now makes the export do a recursive depth-search until a MeshInstance3D is encountered. At this point it stops searching further children of that node and instead proceeds as normal. It will still process all sibling nodes for more items, just stop the search for the children of that specific MeshInstance3D node.

This means users can still add supporting nodes in their export scenes below their mesh instances that are not exported. E.g. the source geometry to bake the navigation mesh that gets exported. This also means very convoluted and exaggerated tree setups like this now can work in the MeshLibrary export:

meshlib_tree_export

As far as compatibility is concerned I think the only way that things break with this recursive search is if users had a MeshInstance3D in their scenes that was already not exported correctly. If exported now this would obviously change the id numbers of the MeshLibrary. I would assume that any dev worth their salt already fixed their export scenes if they encountered such an export bug with a missing mesh so this is imo only a hypothetical concern and they can easy fix it by moving that suspicious node at the end of the tree so it receives an unused id.

…nodes

Makes MeshLibrary export do recursive depth-search for MeshInstance3D nodes.
Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

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

seems good

@akien-mga akien-mga merged commit eac2091 into godotengine:master Feb 7, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@smix8 smix8 deleted the meshlib_export_recursive branch February 9, 2024 09:10
@WebFreak001
Copy link

how would one make a group of disabled/non-importing meshes now? My use-case: I have a large group of imported scenes that I didn't inherit (and don't want to inherit manually anyway) and use a @tool script to duplicate the meshes from each scene into my single MeshLibrary scene + then auto-generate trimesh colliders, so that I don't have to do it for the hundreds of them manually.

Previously I would have been able to just have all my source meshes/scenes inside 1/2 deep nodes to skip them but now that no longer seems to be possible.

Would a quick hot-fix to skip over nodes that have e.g. process_mode set to disabled be accepted for that or should I create a full proposal?

@citrasusanto
Copy link

citrasusanto commented Jul 8, 2024

Hello, iam using Godot 4.2.2 in Pop OS 22.04 and i still experience the silent mesh not being added into the mesh libraries.

https://kenney.nl/assets/tower-defense-kit

I downloaded the assets from here. assets from (28/08/2019)
After i downloaded and manually removed the node tempParent, i showed up after exported however there are some errors on the other tile such as tile with tree where the tree is being ignored so only the tile showed up after exported.

image

Sorry if the fix will be patched for Godot 4.3 above because iam not sure when this will be patched.

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