Found fix for texture exporting #134
Conversation
| @@ -96,18 +96,21 @@ public void SaveGLTFandBin(string path, string fileName) | |||
| gltfFile.Close(); | |||
| binFile.Close(); | |||
| #endif | |||
|
|
|||
| GL.sRGBWrite = true; | |||
There was a problem hiding this comment.
Good find! Though really, these two lines should be the only content of this PR. None of the other changes should be included, please strip them.
There was a problem hiding this comment.
Thanks! I wanted to make a PR ASAP to people can look it at it, but I am planning on making a cleaner one as soon as possible, with only the requested changes included. Expect one within this week :)
|
Now my fork shouldn't have conflicts with the base repo. I only edited the Exporter script. Please try exporting some models and see if the color space problem is still there. FYI, these are the texture types that are in sRGB/Gamma space: |
|
@justinctlam I also liked the idea you had in the PR. My fix here only addresses Color space issues. So the whole idea of the ImageWrapper doesn't need to be there, as long as we can specify what textures are in what color space, we should be fine |
|
@ziedbha Sounds good : ) |
|
@justinctlam checkout the newest version of this PR. I integrated changes in commit d72c74a |
| enum TextureMapType | ||
| { | ||
| //sRGB | ||
| Main, |
There was a problem hiding this comment.
You've got some odd spacing issues in your changes. I think this project uses tabs.
There was a problem hiding this comment.
That's weird, I'll fix that!
| @@ -1 +1 @@ | |||
| m_EditorVersion: 5.6.5f1 | |||
| m_EditorVersion: 5.6.1f1 | |||
There was a problem hiding this comment.
You should probably revert this. It's going back in version?
There was a problem hiding this comment.
This was back in the first PR I made, which wasn't organized at all. My bad. There is probably more stuff to revert (only a couple of scripts should be edited...)
|
|
||
| namespace UnityGLTF | ||
| { | ||
| public class GLTFSceneExporter | ||
| { | ||
| enum TextureMapType |
There was a problem hiding this comment.
Why not add a Boolean that indicates whether the image is sRGB or not instead of a type and then a switch to determine it?
There was a problem hiding this comment.
This was part of @justinctlam's changes. I just didn't want to add more to the code, but I can definitely do that. Thanks for pointing that out!
There was a problem hiding this comment.
I don't follow how my changes are related to sRGB?
There was a problem hiding this comment.
They do not, which is why I added it. We need to export textures in the proper color space, or they won't look right.
| exportTexture.ReadPixels(new Rect(0, 0, renderTexture.width, renderTexture.height), 0, 0); | ||
| exportTexture.Apply(); | ||
| File.WriteAllBytes(Path.Combine(path, image.name + ".png"), exportTexture.EncodeToPNG()); | ||
| private void exportImages(string outputPath) |
There was a problem hiding this comment.
minor coding convention issue: functions are all supposed to be upper camel case.
| } | ||
|
|
||
| if (material.HasProperty("_Roughness")) |
There was a problem hiding this comment.
newer versions of Unity uses _Roughness instead. I think you will break that code if you make this change.
There was a problem hiding this comment.
Not sure what happened there, will fix it.
There was a problem hiding this comment.
Update: since I integrated PR #133 with this one, it pulled changes in that PR too. I think @justinctlam might have some explanation as to why he took out the _Roughness factor
There was a problem hiding this comment.
Which Unity uses _Roughness? I am on Unity 2017.3.1f1. I looked at the Standard.shader and StandardRoughness.shader and I don't see the property. Can you point me to it?
There was a problem hiding this comment.
You are right. This is my mistake. So, this never worked before then, right?
|
I tried exporting with this PR that includes @justinctlam changes and my changes, and these are the results I am getting. I attached an exported BoomBox, Lantern, and Corset, which are 1-1 with the original models. |
|
What happened to setting GL.sRGBWrite? |
|
For some reason, removing the Apply() line after the ReadPixels() line fixed it. After removing that line, I noticed that the GL.sRGBWrite line didn't matter anymore. I'm still trying to understand why, but this version gave me the correct results. EDIT: It might be because of the updateMipmaps argument, which defaults to true. https://docs.unity3d.com/ScriptReference/Texture2D.Apply.html |
|
Put this on hold until Monday, I found a bug I need to fix! |
|
Let me know where you are ready for this to get checked in. Also how your work relate to the work that is being done in PR #133 ? |
|
@blgrossMS will ping you when it's ready! The changes are simply to use the Blit method we have right now to export the images out. PR #133 did not use that method (and needed many passes on the pixels), so it was slower. |
|
@blgrossMS you can ignore this PR. However, please check PR #152 |
I found a fix for the texture exporting, specifically for the bug that had the baseColor be too bright or too dark. The idea is to disable/enable sRGB conversion during the blit operation for each image exported. This is a test run and seems to work, but the most important change I'm working on is to distinguish between Metallic and Base Color (need separate treatment for sRGB for each).