Added material indices. #28
Conversation
There was a problem hiding this comment.
Hey. Thanks for submitting this patch! Looks good.
Left a small request around adding a test.
We should also either update an existing test or add a new test to your Blender export tests that confirms that we are exporting material indices properly.
I would suggest where to add the test but I haven't touched this in over a year so I don't remember.. So whichever you think makes more sense is fine!
https://github.com/chinedufn/landon/tree/master/crates/blender-export-test/src/tests
You can run cargo test --all to confirm that tests pass for you locally?
Then I can run CI on this PR and get your code landed. We can publish it as a breaking change.
Thanks!
| res | ||
| } | ||
|
|
||
| pub fn materials_vec(&self) -> &Vec<PrincipledBSDF> { |
There was a problem hiding this comment.
You can just make this materials since this is already a breaking change anyway
| @@ -93,12 +92,20 @@ impl BlenderMesh { | |||
| } | |||
|
|
|||
| /// A map of material name to the material's data | |||
| pub fn materials(&self) -> &HashMap<String, PrincipledBSDF> { | |||
| pub fn materials(&self) -> HashMap<String, PrincipledBSDF> { | |||
There was a problem hiding this comment.
Can make this
pub fn material_with_name(&self, name: &str) -> Option<&PrincipledBSDF> { ... }
and then just iterate the vec to return the name. I'd imagine that ~all models have a small enough number of materials that iterating O(N) on the vec is fine.
| @@ -73,6 +73,9 @@ impl BlenderMesh { | |||
| let mut expanded_normals = vec![]; | |||
| expanded_normals.resize((largest_vert_id + 1) * 3, EASILY_RECOGNIZABLE_NUMBER); | |||
|
|
|||
| let mut expanded_material_index = vec![]; | |||
| expanded_material_index.resize( largest_vert_id +1, 0u16); | |||
There was a problem hiding this comment.
Mind using the EASILY_RECOGNIZABLE_NUMBER. Meant to help more easily see that data is wrong. Which will help in the future this crate gets heavily refactored..
| @@ -412,6 +423,7 @@ impl DerefMut for EncounteredIndexCombinations { | |||
|
|
|||
There was a problem hiding this comment.
Can we add a test in the tests module at the bottom that confirms that expanding material indices works properly for a mesh with more than one material
| @@ -52,6 +53,9 @@ impl Vertex { | |||
| self.uv | |||
| } | |||
|
|
|||
| /// The index of material associated with face | |||
| pub fn material_index(&self) -> u16 { self.material_index } | |||
| @@ -101,6 +102,7 @@ def execute(self, context): | |||
| for face in mesh.data.polygons: | |||
| num_vertices_in_face = len(face.vertices) | |||
| mesh_json['attribs']['vertices_in_each_face'].append(num_vertices_in_face) | |||
| mesh_json['attribs']['material_index'].append(face.material_index) | |||
There were no material indices attachments, I've added some.
Alas! There is no backward compatibility. The reason is that initially it was considered that blender mesh has a map which maps attached materials names to instances. But that's wrong! Materials attachment is indexed array rather than a map.