google / filament

Filament is a real-time physically based rendering engine for Android, iOS, Windows, Linux, macOS, and WebGL2
https://google.github.io/filament/
Apache License 2.0
17.35k stars 1.83k forks source link

Fix for normal generation does not work for UBERSHADER in gltf_viewer app #7905

Closed mbalajee closed 2 weeks ago

mbalajee commented 3 weeks ago

Describe the bug Fix for proper normal generation does not work for UBSERSHADER. This suggestion works for JITSHADER (default in gltf_viewer.cpp sample) but it doesn't work for UBERSHADER. Reason I want this to work for ubsershader is because we want to apply the fix for Android which only has Ubershader.java. Currently the fix is supported only for desktop app.

Would the possible fix for Android be creating an equivalent Jitshader.java? Not sure why android only has ubershader & the benefits of one over the other. Any help to have the fix for proper normal generation in Android would be helpful. I tried to apply the fix for Android but not working

To Reproduce Steps to reproduce the behavior: Run the sample gltf_viewer.cpp app with this suggested change

Expected behavior Model should appear normally with UBERSHADER

Screenshot 2024-06-04 at 11 40 36 PM

Screenshots Actual behavior - Model appears all black with UBSERSHADER

Screenshot 2024-06-04 at 11 33 05 PM

Logs Model: walls.glb.zip

Desktop (please complete the following information):

Smartphone (please complete the following information):

Additional context

pixelflinger commented 3 weeks ago

I think this is a known problem of the previous fix.

mbalajee commented 3 weeks ago

I think this is a known problem of the previous fix.

I see, haven't seen any mention of this being a known issue in the PR description or anywhere in the code. I was also suggested to make some changes in Asset/Resource loader to make use of the desktop fix in Android so I thought this must be a bug

Also would it make sense to use JITSHADER in Android ?

poweifeng commented 2 weeks ago

You mentioned that it's not fixed for you even with #7917?

I just tried your asset (walls.glb) on a macbook, and it looks fine (but did not look fine before the commit). How are you checking the result:

poweifeng commented 2 weeks ago

Here's some notes of what I did

I added the following to samples/gltf_viewer.cpp

diff --git a/samples/gltf_viewer.cpp b/samples/gltf_viewer.cpp
index 6b83316f1..4472cf948 100644
--- a/samples/gltf_viewer.cpp
+++ b/samples/gltf_viewer.cpp
@@ -747,7 +747,10 @@ int main(int argc, char** argv) {
                 createJitShaderProvider(engine, OPTIMIZE_MATERIALS) :
                 createUbershaderProvider(engine, UBERARCHIVE_DEFAULT_DATA, UBERARCHIVE_DEFAULT_SIZE);

-        app.assetLoader = AssetLoader::create({engine, app.materials, app.names });
+        AssetConfigurationExtended ext {""};
+        AssetConfiguration config = {engine, app.materials, app.names };
+        config.ext = &ext;
+        app.assetLoader = AssetLoader::create(config);
         app.mainCamera = &view->getCamera();
         if (filename.isEmpty()) {
             app.asset = app.assetLoader->createAsset(

I ran it with

./out/cmake-debug/samples/gltf_viewer -u walls.glb

The result is

image

mbalajee commented 2 weeks ago

Clean build worked. Also I modified AssetConfigurationExtended::isSupported() to allow Android and it works on Android too (model appears as expected)