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

Allow optional per-vertex normals in PIE format #338

Open
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@inodlite
Copy link
Contributor

commented Apr 27, 2019

This will let artist use whatever normals they need to, allowing some of the things there were long requested on forum without changing much in game engine.

Optional NORMALS directive is expected before POLYGONS. Number after directive tells you how many lines to read, where each line will have three normals in it (one per index). Total number of normals must be equal to triple number of triangle polygons or we revert back to using calculated normals.

This will also calculate and expose tangent + handedness as a new vertex attribute array "vertexTangent" if normals were loaded from a PIE. Shaders must check "hasTangents" uniform before accessing tangent array.

@Forgon2100

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

Please include an update for doc/PIE.md that documents your changes.

Show resolved Hide resolved doc/PIE.md
@Forgon2100

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

Thank you for fixing the documentation.
Once this PR gets accepted, the wiki article PIE_format should also be updated.

@perim

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

Once this PR gets accepted, the wiki article PIE_format should also be updated.

Or deleted.

@inodlite

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

I'm unsure what .md change has to do with C++ analysis failure...

@Forgon2100

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

I'm unsure what .md change has to do with C++ analysis failure...

These builds can fail for many reasons other than your commits.
In this instance, a third-party module is apparently to blame.

Edit: I restarted the LGTM analysis, and it succeeded.

@Forgon2100

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Jorzi and MaNGusT commented on this PR.

@MaNGusT-

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

After some analyzes and tests it's almost ready. Some adds to code required. Also I will post here working shader soon.

@inodlite

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Code was ported from WMIT that does tangent + handedness calculations and exposes that as a new vertex attribute array as "vertexTangent". Calculations are only done if normals were loaded from a PIE and not calculated by WZ. Component shaders should check "hasTangents" uniform to determine if it can access tangent array.

@MaNGusT- has updated shader that is working with test models that were exported using WMIT.

@past-due

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

@inodlite: Could you pull out the (seemingly?) unrelated fixes (e0b6ed9, 90bf0c8) into a separate PR or two?

@past-due past-due added this to the 3.4.0 milestone May 12, 2019

@inodlite inodlite force-pushed the inodlite:pie_normals branch from 78ed3e1 to 2fe562b May 12, 2019

@inodlite

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Those two commits were used to make sure that new functionality is working as expected, but they are not essential. Dropped both.

@MaNGusT-

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

This archive contain prepared mantis model without connectors with 2 different diffuse textures and updated normals texture for fast tests, it replaces viper. Also mod contains shaders, if they look ok then I-Nod can add em to this PR.

@MaNGusT-

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

I'm tired of these fights with inverted sun in wz. We had some "holy-wars" about how handness should be calculated and where. It's needed to allow artists use mirrored uv:s, very useful feature. Non of ways I tried don't eliminate inverts happen here and there because of wz's inverted sun and coordinate system that needs -x compared to "right" +x coordinate we have in normal maps. I just tried to synchronize the chain Blender(3dsmax)->wmit->wz to use in wz and wmit the same shaders but have no success. WMIT needs separate shader without inverts till someone would fix wz's sun and -x factor.
So lets keep it as it is now. It works, it has minimum inverts, in future it will require minimum changes only in shaders if sun will be fixed. It doesn't matter how and where calculate uv:s' handness, it just has to work and works fast.

@inodlite inodlite force-pushed the inodlite:pie_normals branch from 2d41d5e to efc6201 May 20, 2019

@MaNGusT-

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Here are the final polished shaders. Mantis is just a good model for tests because it has a lot of mirrored uv:s.
pieNormals_test4.zip

We can't fix lightPosition. When we touch it then light becomes totally broken. It's really hardcoded. So as a hack we just flip the Y channel(blue) in normal maps code. Except this everything looks and works good. No more changes needed.

@Forgon2100

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

This work is complete as far as I can tell.
Is there any good reason not to merge it?

@past-due

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Is there any good reason not to merge it?

We're in the beta phase of 3.3.0, so we're focusing on bug fixes.

As soon as 3.3.0 is released and we start on 3.4.0 development, we'll look at this feature. (It will require rebasing, as I should have more of vli's gfx_api refactoring to merge as soon as 3.4.0 development is started.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.