Texture Path Preservation #2203
Conversation
|
🔴 Benchmark Regression Detected ➡️ Report |
|
Please change the code format. Other than that, it looks good to me |
| assert len(plane.vgeoms) > 0 | ||
| vgeom = plane.vgeoms[0] | ||
| assert vgeom.vmesh is not None | ||
| assert vgeom.vmesh.surface is not None | ||
|
|
||
| texture = vgeom.vmesh.surface.diffuse_texture | ||
| assert texture is not None | ||
| assert isinstance(texture, gs.textures.ImageTexture) | ||
|
|
||
| # The texture should have both image_path and image_array | ||
| assert texture.image_path is not None | ||
| assert texture.image_array is not None |
There was a problem hiding this comment.
those asserts are not helpful
There was a problem hiding this comment.
I agree. Either redundant or out of scope.
| # Check that the plane's vgeom has a texture with a path | ||
| assert len(plane.vgeoms) > 0 | ||
| vgeom = plane.vgeoms[0] | ||
| assert vgeom.vmesh is not None | ||
| assert vgeom.vmesh.surface is not None | ||
|
|
||
| texture = vgeom.vmesh.surface.diffuse_texture | ||
| assert texture is not None | ||
| assert isinstance(texture, gs.textures.ImageTexture) |
There was a problem hiding this comment.
I don't understand. These checks seem to be completely unrelated to what this test is pretending to check. Either broaden the scope of this unit test (test name, docstring, comments...) or remove these checks.
| color_image = None | ||
| color_image_path = None |
There was a problem hiding this comment.
I would recommend not supporting specifying both and raise an exception if it happens. Then load image files directly inside this method if provided.
| def create_texture(image, factor, encoding, image_path=None): | ||
| if image is not None: | ||
| return gs.textures.ImageTexture(image_array=image, image_color=factor, encoding=encoding) | ||
| return gs.textures.ImageTexture( | ||
| image_array=image, | ||
| image_path=image_path, | ||
| image_color=factor, | ||
| encoding=encoding, | ||
| ) |
There was a problem hiding this comment.
Same. This API is a bit broken I think. What about adding other method called create_texture_from_path or something, that would do the internal plumbing? Because allowing to specify both image and image_path is weird.
| ), | ||
| material=trimesh.visual.material.SimpleMaterial( | ||
| image=Image.open(os.path.join(get_assets_dir(), "textures/checker.png")), | ||
| image_path="textures/checker.png", |
There was a problem hiding this comment.
This looks like dark magic. This is not officially supported by Trimesh from what I know, but maybe I'm wrong.
| if not (self.image_path is None) ^ (self.image_array is None): | ||
| # Allow both image_path and image_array, but require at least one | ||
| if self.image_path is None and self.image_array is None: |
There was a problem hiding this comment.
I don't think it makes sense.
Description
Preserve texture file paths alongside image data so exporters can reference original texture files instead of re-encoding pixel data.
Changes
Changed validation from XOR (exactly one) to "require at least one"
Only load image from disk if image_array is not already provided
Preserve image_path even when image_array is present
Extract image_path from material.kwargs.get("image_path") when loading SimpleMaterial
Pass extracted path to create_texture()
create_texture(): Added image_path parameter, pass to ImageTexture constructor
create_plane(): Store image_path="textures/checker.png" in plane material
surface_uvs_to_trimesh_visual(): Store texture.image_path in material kwargs when converting to trimesh
Added test_plane_texture_path_preservation() to verify planes preserve texture paths