Fix export of metallic and normal texture map. Add support for any shader that can support metallic roughness workflow. #133
Conversation
…ader that can support metallic roughness workflow.
|
Some notes about my changes. I decided to check the properties to determine which material to export instead of using the name is because it will let developers create their own types of shaders that can be used with this system. For example, we are using a shader in the Unity store called Standard Plus which allows for light maps. |
There was a problem hiding this comment.
Please let me know if I understood this correctly: for each texture, you are going in and making sure it is readable and not compressed by essentially accessing the Unity Editor, right? If that's the case, I think it will be really slow to make sure that each texture readable in order to get its pixels. The original idea was to pass it through the GPU and to get the pixels from there. I think I have a fix that can be combined with yours to make texture exporting as fast as it currently is. I'll make a PR.
|
@ziedbha Yes, I took the idea from the SketchFab Unity to GLTF exporter. I thought it would be slow and would think using the GPU to do it would be faster. I haven't had time to figure out how to do it as I'm still learning the Unity APIs. I would love to learn how to do it. Would you be able to update my PR with that enhancement? |
|
I can definitely do that. I'll sync with your PR and add some changes and it will hopefully boost the speed. I saw you checked PR #134 . To explain, it only fixes color space issues since a lot of textures were not converted in the correct color space using the Blit method. Your suggested changes handle that, but might be really slow. So combining the ability to specify shader properties + speed of getting pixels is the best way to approach this. Therefore I think we should combine both of our changes! |
|
@ziedbha I agree. Thank you! I can't wait to learn how to do it better. |
|
Hi, I haven't heard of any feedback other than from @ziedbha. Should I close this PR? Maybe it's not needed? |
| exportTexture.Apply(); | ||
| File.WriteAllBytes(Path.Combine(path, image.name + ".png"), exportTexture.EncodeToPNG()); | ||
| } | ||
| exportImages(path); |
There was a problem hiding this comment.
ExportImages is more the C# style and what we have been using in this code base
| break; | ||
| case IMAGETYPE.RGB: | ||
| case IMAGETYPE.RGBA: | ||
| // inputColor = inputColor; |
There was a problem hiding this comment.
This is removed.
| switch (inputChannel) | ||
| { | ||
| case IMAGETYPE.R: | ||
| inputColor = new Color(inputColor.r, inputColor.r, inputColor.r, inputColor.r); |
There was a problem hiding this comment.
Why are you creating a new Vec4 out of a single float field?
There was a problem hiding this comment.
This function has been removed.
| { | ||
| outputColors[i] = new Color(1.0f, 1.0f, 1.0f); | ||
| } | ||
| addTexturePixels(ref texture, ref outputColors, IMAGETYPE.R, IMAGETYPE.B); |
There was a problem hiding this comment.
is the point of this to swap the R and B channel?
There was a problem hiding this comment.
Removed, the channel swap is done in the shader.
| File.WriteAllBytes(Path.Combine(outputPath, texture.name + ".png"), newtex.EncodeToPNG()); | ||
| } | ||
|
|
||
| private void addTexturePixels(ref Texture2D texture, ref Color[] outputColors, IMAGETYPE inputChannel, IMAGETYPE outputChannel) |
There was a problem hiding this comment.
these functions could all use comments, pretty confused
There was a problem hiding this comment.
These functions are no longer necessary.
| private NormalTextureInfo ExportNormalTextureInfo(UnityEngine.Texture texture, UnityEngine.Material material) | ||
| private NormalTextureInfo ExportNormalTextureInfo( | ||
| UnityEngine.Texture texture, | ||
| TextureMapType textureMapType, |
There was a problem hiding this comment.
Should be fixed now.
| G_INVERT | ||
| } | ||
|
|
||
| enum TextureMapType |
| { | ||
| var textureWidth = texture.width; | ||
| var textureHeight = texture.height; | ||
| Color[] outputColors = new Color[textureWidth * textureHeight]; |
There was a problem hiding this comment.
Just make outputColors an out parameter to the addtexturepixels function and have it create the array. The preinitialization with default values shouldn't be needed if you are filling it all in the calling function
There was a problem hiding this comment.
These are removed, now I am using Graphics.blit.
| outputColors[i] = new Color(1.0f, 1.0f, 1.0f); | ||
| } | ||
| addTexturePixels(ref texture, ref outputColors, IMAGETYPE.R, IMAGETYPE.B); | ||
| addTexturePixels(ref texture, ref outputColors, IMAGETYPE.A, IMAGETYPE.G_INVERT); |
There was a problem hiding this comment.
Why don't you swap all the values you want to in a single pass instead of two passes?
There was a problem hiding this comment.
This is removed, now the swap is done in the shader.
- Use Graphics.blit to speed up texture read/write - Update texture image paths to use full paths to avoid name collision - Don't do anything if user cancels the dialog
|
I've fixed all the feedback. @ziedbha I've updated to use Graphics.blit. Thanks! |
|
Hi! Just wanted to see if there are any more issues I can address for this. At the moment exporting GLTF with pbrmaterials aren't working correctly. |
| @@ -30,6 +33,8 @@ static void ExportScene() | |||
|
|
|||
| var exporter = new GLTFSceneExporter(transforms); | |||
| var path = EditorUtility.OpenFolderPanel("glTF Export Path", "", ""); | |||
| exporter.SaveGLTFandBin(path, scene.name); | |||
| if (path != "") { | |||
| } | ||
| } | ||
|
|
||
| private SceneId ExportScene(string name, Transform[] rootObjTransforms) | ||
| private void ExportMetallicGlossTexture (Texture2D texture, string outputPath) |
| { | ||
| var scene = new Scene(); | ||
| var destRenderTexture = new RenderTexture (texture.width, texture.height, 0, RenderTextureFormat.ARGB32, RenderTextureReadWrite.Linear); | ||
| var shader = Resources.Load ("MetalGlossChannelSwap", typeof(UnityEngine.Shader)) as UnityEngine.Shader; |
There was a problem hiding this comment.
cache the shaders at the start of the exporter instead of doing it here
There was a problem hiding this comment.
Caching the material with shaders
|
|
||
| var exportTexture = new Texture2D (texture.width, texture.height); | ||
| exportTexture.ReadPixels (new Rect (0, 0, destRenderTexture.width, destRenderTexture.height), 0, 0); | ||
| exportTexture.Apply (); |
There was a problem hiding this comment.
I think this will do a CPU and GPU copy even though we just care about the CPU version here to write out. Is there a way to do this without creating an additional texture (like copying the bytes of the destRenderTexture into a file?)
There was a problem hiding this comment.
Looking around, it seems like this is the recommended pattern, add a comment explaining why we create another gpu copy of the data
There was a problem hiding this comment.
I'm not sure why this is the recommended pattern. Is there a better way to do this? I was following the same pattern as before.
| exportTexture.Apply (); | ||
|
|
||
| var finalFilenamePath = ConstructImageFilenamePath (texture, outputPath); | ||
| File.WriteAllBytes (finalFilenamePath, exportTexture.EncodeToPNG ()); |
There was a problem hiding this comment.
This is more a general note for how export to be done than anything that you should do on your review, but we should match the ILoader interface in the importer so that we are using streams in the exporter through a loader like interface and not assuming File I/O or being in control of it here even
There was a problem hiding this comment.
Makes sense. I was just following the existing code already in place.
| { | ||
| var scene = new Scene(); | ||
| var destRenderTexture = new RenderTexture (texture.width, texture.height, 0, RenderTextureFormat.ARGB32, RenderTextureReadWrite.Linear); |
There was a problem hiding this comment.
make your rendertextures with gettemporary so that it will potentially pool them: https://docs.unity3d.com/ScriptReference/RenderTexture.GetTemporary.html
There was a problem hiding this comment.
call release at the end of the function
There was a problem hiding this comment.
Cool tip! thanks!
| File.WriteAllBytes (finalFilenamePath, exportTexture.EncodeToPNG ()); | ||
| } | ||
|
|
||
| private void ExportNormalTexture (Texture2D texture, string outputPath) |
|
@blgrossMS I've updated the PR with the feedback you provided. Thanks again for looking at it! |
|
Hi, just wanted to see if there are any more issues I should address? Thanks! |
|
@justinctlam was testing before merging in, no texture were generated when I did a scene or selection export on the object. I also noticed that an error is thrown if RenderTexture.active is not set to null before release. |
|
@blgrossMS Thanks for looking, I will investigate those issues. |
|
@blgrossMS Do you happen to have a test scene I can look at? The scene I'm using exports textures fine for scene and selection object and I don't see any error thrown. |
|
@blgrossMS I tried to export a scene and selected object on Master build and it also didn't export the textures. Is this something my PR should also fix? |
|
It does not have to. Can you include the scene that you are testing with in the PR (or is it one of the included ones already) just so that I can verify the functionality? |
|
@blgrossMS I am using these free pbr assets from the Unity asset store: If you download them, there is a test scene in it. You can also try out many other free pbr assets on the Unity asset store. They should all work. |
|
This changeset adds dependencies on In my use case, I'm converting OBJs to GLTFs at runtime (and also exporting some additional metadata while doing the conversion -- think json for cesium 3dtiles, but not quite), so 1) I'd like to use this at runtime, and 2) I'd like to find a way to export textures that are not stored inside `/Assets/'. My naive approach was to substitute: This is probably not ideal (doesn't guarantee uniqueness of the texture filename, for instance) but seems like a better option than losing runtime support. Also (minor nitpick) it might be worth moving the shaders from the |
|
@RPSpicer I can look into removing the dependency on UnityEditor. If we are okay with naming collision or have an alternative way to resolve it, I'm happy to hear any feedback. |
|
@justinctlam Ideally we'd want to know the relative path from the input scene/mesh file to each input texture file, but that can't be guaranteed in all cases, for instance my runtime case (where these were loaded into Unity datastructures using another piece of code and now exist in memory). Since there's so many use cases we may end up needing configuration options to meet all needs. As a default, I am biased in favor of "name the exported texture using the Unity I'd advocate for refactoring Editor specific calls over into the |
|
@RPSpicer I added a delegate to decouple the UnityEditor from the runtime exporter. Please see the latest updates. Let me know if this would work for you. |
|
@justinctlam Thank you! This approach works for me, and enables end users to specify their own arbitrary path logic, which seems like a best case for this library. |
|
sorry to bump over this but, nice tool im try something i need with but there are issues with texture not exporting right. i have to export an avatar model to glb thats have a shared material where the textures inputs changes at runtime from some short of list and this dosnt work , even the shoes that has lit urp default not detect the correct textures that has the prefab in runtime, its like if its always pointing to the fbx model or original root of the mesh model and not current materials of the prefab i want to export? sounds confusing can anyone help please ? thanks |
…ader that can support metallic roughness workflow. (KhronosGroup#133) * Fix export of metallic and normal texture map. Add support for any shader that can support metallic roughness workflow. * Addressed feedback - Use Graphics.blit to speed up texture read/write - Update texture image paths to use full paths to avoid name collision - Don't do anything if user cancels the dialog * Revert irrelevant files * Fix indentation * Update accessor for structs and enum * Update formatting * Address feedback * Remove exporter dependency on UnityEditor.
Fix export of metallic and normal texture map. Add support for any shader that can support metallic roughness workflow.