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

Shader rework #42

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

Shader rework #42

wants to merge 48 commits into from

Conversation

BlackYps
Copy link

@BlackYps BlackYps commented Oct 6, 2024

The aim of this PR is to reimplement the game's shaders. The existing implementation in the editor is very convoluted and can't be easily extended to more shaders that we now have in the game.
Additionally people reported lighting differences between the editor and the game.

This rework aims to provide an environment where the shaders of the game can be copy-pasted over to the editor with minimal adjustments to make it easier to spot if everything is working correctly.

During this rework I also implemented the Terrain301 shader, to test if this framework is also feasible for the different inputs that these kind of new shaders need.

The shader toggle has been changed to a text field for now. In the future this should probably be a list or radio buttons to show the available shaders, to be more user-friendly.

@BlackYps
Copy link
Author

Current state:
grafik

This is a bullshit implementation and obviously not what the game does. But it's what the map editor has been doing so far, so we'll keep it for now.
@BlackYps BlackYps marked this pull request as ready for review October 17, 2024 11:28
@BlackYps
Copy link
Author

FaTerrainShader.shader is unused now. It kept it just to be easily able to look up the old code. We could either delete the old file or copy the contents of NewFaTerrainShader.shader into this file.

@Garanas
Copy link
Member

Garanas commented Oct 17, 2024

Can you give me pointers to what you would like me to review? I can not, for example, review the specifics of the shader code. My mathematics is simply not good enough. But I can review the code and the output in general and share my thoughts about that.

@BlackYps
Copy link
Author

I think an in-depth shader code review is not needed. We can verify the shader code by looking at the graphical output and comparing it to the game. In other words, some testing of the functionality would be nice.
You can also compare the shader code with the game shaders from here https://github.com/FAForever/fa/blob/develop/effects/terrain.fx and give your opinion if my goal of easily being able to compare game and editor shaders succeded.
Ideally, you should be able to compare individual functions with a diff tool and only encounter differences that make sense. Comparing whole files is obviously not possible, but being able to compare on the function level was the goal.

As the current map generator maps are a bit bugged with the editor, you can use this map that I created with the changes from FAForever/Neroxis-Map-Generator#423 to properly test out the Terrain301 shader.
neroxis_map_generator_snapshot_sgg3i4vg2xmuu_bygaeaaiaaaaclzw.zip

@BlackYps
Copy link
Author

I originally planned to only change the terrain shader here and to make a separate PR for the water and props shader. But I noticed that it's probaby less effort for everyone to have everything at once. This way you only have to test once and everything already looks like it should.
Note that the diffs become a bit unwieldly, because I moved the orginally separate files with the new shader code into the old ones in the second-to-last commit. There is no reason to keep the old, unused code. If you want to have a cleaner look at the files (again, no deep review required there, we can simply verify the visual result), you can either view the file directly or exclude that commit in the diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants