AwesomesauceLabs / piglet-viewer

Demo viewer app for Piglet glTF Importer
MIT License
6 stars 4 forks source link

Texture artifacts with runtime importer #4

Closed oprogramadorreal closed 2 years ago

oprogramadorreal commented 2 years ago

Hello! I've tried to contact you in your e-mail. I've sent you the GLTF file that I'm trying to import. The issue that I'm having is the following:

unnamed

On the left, the GLTF was imported using the editor importer. On the right, the same GLTF file was imported using the runtime importer. I see both materials use the same shader. The difference seems to be in the textures. The textures generated at runtime do not seem right but I'm not sure what is the problem with them.

I'd like the runtime importer to provide the same result as the editor importer. I'd appreciate any help.

oprogramadorreal commented 2 years ago

This is the GLTF file that I'm trying to import: Gltf.zip

benvvalk commented 2 years ago

Hi @oprogramadorreal!

Sorry, I did not receive your e-mail for some reason. (My business email address is awesomesaucelabs@gmail.com).

Thanks for providing the test file. I have not seen that strange banding problem with the textures before.

One possible cause is that the Editor and runtime textures are using different texture compression methods. Editor imports use DXT compression, whereas runtime import use raw uncompressed RGBA. I will try experimenting compression options in the Texture Import Settings in the Editor, to see if that is the cause.

Another possible cause is that Editor imports create mipmaps, whereas (unfortunately) runtime imports do not create mipmaps, due to limitations of the UnityWebRequestTexture API.

I will have a closer look at your file later today.

Also, no problem if you want a refund. Just let me know the email address for your PayPal account and the original amount you paid for Piglet.

oprogramadorreal commented 2 years ago

Hi @benvvalk Thanks for the reply. I don't want a refund at all but I'd love to see this issue fixed. Hope we can find a solution. I'm gonna wait for your investigation. Thanks again!

benvvalk commented 2 years ago

Hi @oprogramadorreal,

The banding patterns (a.k.a. Moiré pattern) occur because Piglet doesn't create mipmaps during runtime imports. You can recreate the same banding effect with the Editor-imported model if you uncheck Generate Mipmaps in the Texture Settings, as shown in the following screenshot.

disable-mipmaps

Actually the whole purpose of mipmaps is to fix this kind of banding/aliasing problem. For example, there are similar banding patterns in the first example image from the MipMap article on Wikipedia.

So why doesn't Piglet create mipmaps during runtime imports?

So far I have been using UnityWebRequestTexture to load textures at runtime, which doesn't have an option to create mipmaps.

It is possible to use Texture2D.LoadImage instead, which does have an option to create mipmaps. However, that method blocks the main Unity thread during PNG/JPG compression, which is really bad for performance. (UnityWebRequestTexture performs PNG/JPG decompression on a background thread.)

I'll try to find some workarounds for creating mipmaps at runtime.

oprogramadorreal commented 2 years ago

Hey @benvvalk thanks a lot! To generate mipmaps I was trying to do some of what they say in this thread: https://forum.unity.com/threads/generate-mipmaps-at-runtime-for-a-texture-loaded-with-unitywebrequest.644842/

I tried to do something like (at GltfImporter):

var t = _imported.Textures[i];

var newTexture = new Texture2D(t.width, t.height, t.format, true);
newTexture.LoadRawTextureData(t.GetRawTextureData<byte>());
newTexture.Apply(true);

_imported.Textures[i] = newTexture;

and call this at OnCompleted. However, it seems these textures cannot be read yet because I get this exception (when calling GetRawTextureData): UnityException: Texture 'texture_0' is not readable, the texture memory can not be accessed from scripts. You can make the texture readable in the Texture Import Settings.

But I'm completely unsure if this would work.

benvvalk commented 2 years ago

Hi @oprogramadorreal :-)

Sorry, I've been meaning to take another look at this issue.

I think your approach makes sense. Try adding this line before your code:

_imported.Textures[i] = TextureUtil.GetReadableTexture(_imported.Textures[i]);

Note: TheTextureUtil.GetReadableTexture method relies on RenderTexture, and in the past I've had trouble getting RenderTextures to work correctly in WebGL builds. More specifically, I could not get my RenderTexture code to work correctly in Chrome on Windows 10. Just wanted to mention it in case you are developing for WebGL.

benvvalk commented 2 years ago

Another possible workaround is replace UnityWebRequestTexture with Texture2D.LoadImage, using the following patch (attached as piglet-loadimage.txt):

diff --git a/Assets/Piglet/Scripts/Texture/TextureUtil.cs b/Assets/Piglet/Scripts/Texture/TextureUtil.cs
index 9bba73f..3d9362c 100644
--- a/Assets/Piglet/Scripts/Texture/TextureUtil.cs
+++ b/Assets/Piglet/Scripts/Texture/TextureUtil.cs
@@ -388,18 +388,15 @@ namespace Piglet
        /// </summary>
        static public IEnumerable<(YieldType, Texture2D)> LoadTexturePngOrJpg(Uri uri)
        {
-           var request = UnityWebRequestTexture.GetTexture(uri, true);
-           request.SendWebRequest();
-
-           while (!request.isDone)
-               yield return (YieldType.Blocked, null);
-
-           if( request.HasError())
-               throw new Exception( string.Format(
-                   "failed to load image URI {0}: {1}",
-                   uri, request.error ) );
+           byte[] imageData = null;
+           foreach (var (yieldType, data) in UriUtil.ReadAllBytesEnum(uri))
+           {
+               imageData = data;
+               yield return (yieldType, null);
+           }

-           var texture = DownloadHandlerTexture.GetContent(request);
+           var texture = new Texture2D(1, 1, TextureFormat.ARGB32, true, false);
+           texture.LoadImage(imageData, true);

            yield return (YieldType.Continue, texture);
        }

I briefly tested the patch with your example Gltf.zip file and it does seem to improve the banding issue.

benvvalk commented 2 years ago

Another issue I noticed is that your Gltf.zip file uses the KHR_texture_transform glTF extension, which I haven't implemented in Piglet yet.

In the case of your glTF file, the consequence is that the texture is scaled incorrectly (stretched by a factor of 3x) and that probably makes the banding problem look even worse.

It's on my TODO list to implement KHR_texture_transform because it's a commonly used glTF extension and I've been getting a lot of bug reports about it.

oprogramadorreal commented 2 years ago

@benvvalk thanks a lot for the suggestions. They indeed helped to mitigate the issue. It would be great if you could add some of theses solutions into a next version of Piglet.

Regarding the support for KHR_texture_transform extension, would you have any idea of when you'll tackle this? I have seen other issues that may be related to this as you mentioned.

I have approaching deadlines and other things to do in my project but since GLTF importing is a critical part of it I would consider try to solve these issues by myself, unless you say you are about to solve them. :)

It would be nice if you could share some roadmap or just give me an idea of your plans for Piglet.

Anyway thank you for the support you already gave me so far!

benvvalk commented 2 years ago

Hi @oprogramadorreal,

You're welcome and I'm glad I could help!

Yes, for the next Piglet version I will add an option to use Texture2D.LoadImage instead of UnityWebRequestTexture (probably controlled by a compile time define). Depending on the application, people might be willing to accept the downgraded performance of using LoadImage (i.e. FPS drops) in order to have mipmaps. It is a shame that Unity doesn't provide better APIs for runtime texture loading.

Re: KHR_texture_transform. Yes, I plan to add support in the next Piglet version (1.3.7), which will probably be released in mid-to-late May. (Even though I work on Piglet full time, my release cycles are quite slow because I do a lot of manual testing, across many platforms and Unity versions.) Before the release, I will send you an early implementation so you don't have to wait for all the testing.

Re: deadlines/schedules. I'm currently available for paid contract work, if you need certain fixes/features implemented before a deadline. (Please email me privately at awesomesaucelabs@gmail.com to negotiate rates and deadlines.) Otherwise, I make no promises about timelines (only estimates).

Re: Piglet roadmap. In the long term, I want Piglet to be a feature-complete glTF importer/exporter with support for the most commonly-used glTF extensions. It is a big project and I may be working on it for years! In the short term, I keep track of every requested fix/feature (several per day), and I just work on the fix/feature that I believe will give the most benefit-per-time-spent, one at a time. In order of descending priority, my current items are:

benvvalk commented 2 years ago

@oprogramadorreal,

I forgot to say: I'd be happy to provide (free) guidance about the Piglet code, for any features you decide to add yourself. In the case of KHR_texture_transform, I would advise waiting unless you desperately need it very soon (e.g. < 2 weeks from now).

oprogramadorreal commented 2 years ago

Thanks for sharing your plans @benvvalk, that's really nice. Also good to know that we can use your free guidance or hire you if we need something faster. For now we gonna keep using Piglet the way it is and wait for the next updates. Hope we can contribute a little to its growth with our feedbacks.

benvvalk commented 2 years ago

Hi @oprogramadorreal,

Just an update about runtime mipmaps.

I did some experimenting with the LoadRawTextureData approach you suggested in your comment above. It was a good idea and it causes less stalling of the main Unity thread compared to LoadImage (or none at all). I will include it in the next Piglet release (still a few weeks away), but for your reference I've also attached the changes as loadraw-patch.txt.

I had to make some minor modifications to make your code work, by following the suggestion provided in this Unity Forum post.

Also, I did some profiling of loadimage/loadraw mipmap-loading methods in order to help me answer the following questions:

  1. Should I enable mipmaps by default in the next Piglet version? No, it will have to be enabled with an option. Even when using LoadRawTextureData there is too much performance overhead and I don't want to give my existing users any bad surprises.
  2. Which has better performance: the LoadImage method or LoadRawTexture method? The LoadRawTexture method is generally better.

With respect to total wallclock time for loading textures, the LoadImage and LoadTexture methods are similar. Roughly speaking, they both double the texture loading wallclock time:

wallclock

However, I also measured the individual frame update times, and in that case the LoadRawTexture method was much better than LoadImage. However, no mipmaps is still the best:

frametimes

Note: I made the histograms by aggregating results across 5 runs (import the same glTF file 5 times). The LoadImage results are even worse than they appear, because the "100" histogram bin is an overflow bin that contains all values >= 100.

benvvalk commented 2 years ago

Oops... I discovered a big error in my histogram-plotting script.

I've included the corrected plot below, with the y-axis (count) on a logarithmic scale this time. The conclusions are still the same, though:

(1) Mipmaps should be disabled by default during runtime glTF imports. (2) The LoadRawTextureData method is better than the LoadImage method.

movenext-logy

AwesomesauceLabs commented 2 years ago

@oprogramadorreal

Just a heads up: I finally released Piglet 1.3.7 today, which adds a new CreateMipmaps option (see Runtime Import Options), and also adds support for KHR_texture_transform.

Sorry for the long wait and I hope it is still useful for you!

oprogramadorreal commented 2 years ago

@benvvalk thanks for the heads up and for the new version. It will sure be useful for me. I'll check it out. Thanks!