Allow converting from Mesh to ImporterMesh#113202
Conversation
Calinou
left a comment
Calinou
left a comment
There was a problem hiding this comment.
Code looks good to me.
It may be worth adding unit tests for this in the future. I noticed we don't have test_mesh.h or test_importer_mesh.h yet though, only test_arraymesh.h and test_triangle_mesh.h.
|
A round trip test could be ideal, making sure all of the properties are present after converting to and from ImporterMesh. |
|
I attempted to start to write a minimal test: TEST_CASE("[ImporterMesh] Round-trip from BoxMesh to ImporterMesh to ArrayMesh") {
Ref<BoxMesh> box_mesh;
box_mesh.instantiate();
//Ref<ImporterMesh> importer_mesh = ImporterMesh::from_mesh(box_mesh);
}But it fails to construct the BoxMesh on this check: |
lyuma
left a comment
lyuma
left a comment
There was a problem hiding this comment.
Looks good.
I would assume that the unit tests would use a dummy driver for the RenderingServer, and also that the dummy driver would support primitive mesh types... but I don't know enough about the testing infrastructure to understand why this would fail nor how to fix it. So my suggestion is to merge this without adding the round trip test.
github-project-automation
bot
moved this from Ready for review
to Done
in Asset Pipeline Issue Triage
|
Thanks! |
7 participants
ImporterMesh is designed primarily for use during the import process, as the name implies. However, the GLTF module needs to do the reverse as well, when exporting a glTF file from Godot.
In master, GLTFDocument contained duplicate code for converting ImporterMesh to Mesh. I could just deduplicate this within GLTFDocument, but in the near future, I will need to reference this function outside of GLTFDocument (specifically,
GLTFDocumentExtensionMultiMeshin PR #112866). This PR moves this functionality to ImporterMesh, and exposes it.This PR also fixes a minor bug where the material name was used as the surface name, taking priority over the... actual surface name. To make this clearer, I also renamed the
ImporterMesh::add_surfaceparameterp_nametop_surface_nameto explicitly indicate that this should be the surface name and not the material name.