MikePopoloski / SharpBgfx

C# bindings for the bgfx graphics library
MIT License
169 stars 32 forks source link

Compiling Shaders via MSBuild #17

Closed smack0007 closed 6 years ago

smack0007 commented 7 years ago

Finally had some time to get around to this. Opening the pull request already to start a dialog.

Currently I've only modified 01-Cubes but will of course update all the other projects as soon as we agree this is headed in the right direction.

The output for the shaders are now in the same directory as the output for the project under "bin\{platform}".

TODO:

MikePopoloski commented 6 years ago

Sorry it too me so long to respond. This looks pretty good to me; nice and simple. From your list of TODOs, the most important I think would be getting all the projects updated and then making sure the shaders don't get recompiled on every single build in the IDE.

smack0007 commented 6 years ago

I moved shader header files (common.sh, shaderlib.sh, bgfx_compute.sh, bgfx_shader.sh) as I would like the shader code to be the exact same as in the bgfx repo.

I put common.sh and shaderlib.sh in "examples\Common" to match the bgfx repo.

For bgfx_compute.sh and bgfx_shader.sh I wasn't sure so I created a new directory "include". In the bgfx repo they are in the "src" directory but I feel like that is wrong for us. Let me know if you would like me to put them somewhere else.

MikePopoloski commented 6 years ago

Adding the include directory seems fine to me. I'm also fine with moving the other headers around too.

smack0007 commented 6 years ago

All the example projects are now using the SharpBgfx.targets file to compile their shaders.

smack0007 commented 6 years ago

After a major refactoring here are the new features:

smack0007 commented 6 years ago
  • Changing a shader source file recompiles the shaders when only doing a Build (this didn't work before, only worked on Rebuild).

Hmm, after playing around with it a little more this still doesn't seem to work 100% correctly. Rebuild always works of course but will look into it further when I have more time.

smack0007 commented 6 years ago

Adding <DisableFastUpToDateCheck>true</DisableFastUpToDateCheck> to the SharpBgfx.targets seems to have fixed the issue. This instructs Visual Studio to always start MSBuild when instructed to build the project.

More information here: https://stackoverflow.com/questions/5240820/how-does-visual-studio-2010-hosts-msbuild-for-c-projects

MikePopoloski commented 6 years ago

Let me know when you want me to try it out / merge it. Not sure if you're planning to check off your whole list before merging?

smack0007 commented 6 years ago

The last thing I want to do is store the information for profiles in an ItemGroup. This has the benefit of making the profiles configurable and also reducing redundancy. I think I'll have time to get to that tomorrow. After that the feature is done for me except documentation.

smack0007 commented 6 years ago

I gave it a shot yesterday and while I think I could eventually have made it work, it would have made the script to complicated that I'm now sure I would have understood it myself in 3 months time. It's important to me that the script is somewhat understandable for people who aren't MSBuild ninjas. So I'm going to say that you can test the script in it's current form and if you don't see any problems then merge it.

The problem comes from having 1 (m) inputs producing 2 (n) outputs. Right now it's easy enough to express that in MSBuild because the outputs (dx11/glsl) are hard coded. As soon as I try to make the outputs configurable, producing and using the m x n ItemGroup becomes super complicated.

I came to the conclusion yesterday that it would probably make sense to convert part of the script to an inline task: https://msdn.microsoft.com/en-us/library/dd722601.aspx. This would have the advantage of the most complicated part of the script would be written in C#. I would do this in a different pull request though.

MikePopoloski commented 6 years ago

I downloaded and played around with it and it seems to work pretty smoothly. Nice work, thanks for the contribution. I've merged it into master.