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 lighting on models that use normal maps #6073

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

ghost
Copy link

@ghost ghost commented Feb 14, 2025

The lighting on the models that use a normal map were not looking correct and the normal map had no effect on lighting.
I setup a test shader that did a dump of a_tangent and it was always (0, 0, 0) which explained why the normal calc in the fragment shader was incorrect. I then did a dump of a_uv1 and it had the tangent data. I swapped the locations of a_tangent and a_uv1 in attributes.glsl and the lighting on normal mapped models now looks correct , to me :).

After some digging I found that attributes.glsl and Program.cpp have tangent at location 5 but the vertex buffer layout in VertexBufferGL.cpp (line 23) has tangent at index 4.

So by putting a_tangent in location 4 the lighting is now correct for models using a normal map

@ghost
Copy link
Author

ghost commented Feb 14, 2025

Some pics after the fix:

Mars Landing early morning
marsLanding

Mars Sunrise
marsSunrise

Malabar
malabar

Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching this issue!

Because the meaning of ATTRIB_TANGENT is intended to be the numeric value 5, I'd prefer you change VertexBufferGL.cpp line 23 to return the proper attribute index instead. It's slightly simpler that way and shares the same value across both shaders and C++ source.

@ghost ghost force-pushed the model-lighting-fix branch from 369310a to c301a04 Compare February 14, 2025 19:31
@ghost
Copy link
Author

ghost commented Feb 14, 2025

Yep, makes sense. Done.

@ghost ghost force-pushed the model-lighting-fix branch from c301a04 to 26ace92 Compare February 14, 2025 19:37
@ghost ghost requested a review from sturnclaw February 14, 2025 19:38
@sturnclaw
Copy link
Member

As a general git-etiquette thing, we prefer to avoid mentioning issue numbers and @-usernames in commit messages; every time the commit in question is pushed (including to other users' forks, etc.) Github triggers a notification of the mentioned issue/user.

Otherwise the code looks quite simple now! 😉

- lighting on models that use a normal map were incorrect and normal map had no effect on lighting
- a_tangent was always (0, 0, 0) but a_uv1 had tangent data
- attribs.glsl and Program.cpp have tangent at location 5 but the vertex buffer layout has tangent at index 4.
- ATTRIB_TANGENT changed to index 5 in gett_attrib_index() in VertexBufferGL.cpp
@ghost ghost force-pushed the model-lighting-fix branch from 26ace92 to c7555c2 Compare February 15, 2025 00:29
@fluffyfreak fluffyfreak self-requested a review February 15, 2025 12:36
Copy link
Contributor

@fluffyfreak fluffyfreak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved 👍 one character bug fixes are the best 😁

@fluffyfreak
Copy link
Contributor

I gave it a few days in case anyone wanted to merge or comment, merging now

@fluffyfreak fluffyfreak merged commit 33158a5 into pioneerspacesim:master Feb 18, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants