Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mbalajee opened this issue Jun 5, 2024 · 5 comments · Fixed by #7917
Closed

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

mbalajee opened this issue Jun 5, 2024 · 5 comments · Fixed by #7917
Assignees
Labels
bug Something isn't working gltf Specific to glTF support

Comments

@mbalajee
Copy link
Contributor

mbalajee commented Jun 5, 2024

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):

  • OS: Mac (reproducible in any device)
  • GPU: [e.g. NVIDIA GTX 1080]
  • Backend: Metal

Smartphone (please complete the following information):

  • Device: [e.g. Pixel 2]
  • OS: [e.g. Android Pie 9.0]

Additional context

@pixelflinger pixelflinger added bug Something isn't working gltf Specific to glTF support labels Jun 5, 2024
@pixelflinger
Copy link
Collaborator

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

@mbalajee
Copy link
Contributor Author

mbalajee commented Jun 5, 2024

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
Copy link
Contributor

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:

  • For example, are you building from source (git log to check the latest commit)?
  • What platform/device are you testing on?

@poweifeng
Copy link
Contributor

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working gltf Specific to glTF support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants