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

Added a shadertoy example #314

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

Added a shadertoy example #314

wants to merge 5 commits into from

Conversation

dov
Copy link
Contributor

@dov dov commented Jul 23, 2024

This commit adds a shadertoy example. It runs a shadertoy shader from a file given on the command line. If no shader is given, then a default shader is run. The shadertoy directory contain another couple of example shaders. In addition a lot of the shaders from shadertoy can be run. (If a shader from shadertoy cannot be run, then it is bug that should be fixed).

@robertosfield
Copy link
Collaborator

Thanks for the example. I have done a quick code review and broadly it all looks good. I am heading out for the day soon so can't try it out right away.

One thing I was curious about was the .shy shader extension, I haven't seen that done before. GLSL shaders would normally be .vert, .geom, .frag or .glsl etc.

@dov
Copy link
Contributor Author

dov commented Jul 24, 2024

Thanks for the feedback! Regarding the "shy" extension (as in SHader toY), I made it up on my own to differentiate it from the "real" glsl shaders. Currently .shy files are turned into frag files within vsgshadertoy simply by wrapping them with some template code.

I'd be happy if you could instruct me on how I can remove the MVP push constants, as well as other redundant code from the example.

@robertosfield
Copy link
Collaborator

To get the ball rolling I have merged the example as is with a dov-shadertoy branch of vsgExamples:

https://github.com/vsg-dev/vsgExamples/tree/dov-shadertoy

I tried the shaders out an then all work :-)

toyShader = readFile(argv[1]);
string title;
// extract the filename from the path
auto pos = string(argv[1]).find_last_of("/");
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work on Windows (which uses '')? Might be better to use std::filesystem's filename() function? (std::filesystem is C++17 so should be fine with VSG?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Won't work. I'll fix it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

std::filesystem is officially part of C++17 so "should" be fine, except it's gcc and clang have only recently got there act together and properly support it out of the box, older versions don't properly support std::filesystem, it's why I had to resort to writing a vsg::Path class that is equivalent to std::filesystem::path.

The VSG has various path manipulator convenience functions:

https://github.com/vsg-dev/VulkanSceneGraph/blob/master/include/vsg/io/FileSystem.h
https://github.com/vsg-dev/VulkanSceneGraph/blob/master/include/vsg/io/Path.h#L218

Ironically these are more easily portable than the mess that was/is std::filesystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I rewrote the code to use vsg::Path() instead of std::last_of("/")

@robertosfield
Copy link
Collaborator

It should be possible to pass the vsg::Path to the code that reads the shader file, rather than have it use std::string. vsg::Path handles the Windows wide string filename paths just file std::filesystem::path.

@dov
Copy link
Contributor Author

dov commented Jul 29, 2024

I just hardcoded the vertices coordinates in the vertex shader like in:

https://www.saschawillems.de/blog/2016/08/13/vulkan-tutorial-on-rendering-a-fullscreen-quad-without-buffers/

This allowed me to remove the vertex and index buffers, and the binding descriptors for the vertices.

It's less general as it doesn't draw an arbitrary mesh any longer, but it is an example of minimalism.

dov added 4 commits October 11, 2024 10:41
- Use vsg::Path for paths instead of strings
- Got rid of the redundant push constant
- Removed `using std` as that is not the convention in vsgExamples
- Changed the title so that it always display "vsgshadertoy"
- Removed the vertex and the index buffers
- Removed their respective descriptors
- Use single large triangle for covering quad like in https://www.saschawillems.de/blog/2016/08/13/vulkan-tutorial-on-rendering-a-fullscreen-quad-without-buffers/
- Change mouse behavior to react only if mouse is pressed
@dov
Copy link
Contributor Author

dov commented Oct 11, 2024

@robertosfield Is there anything that I can do to speed up the merging of this branch?

@robertosfield
Copy link
Collaborator

I have done another quick code review, and feel that I'd rather we have ordinary glsl shaders rather than a custom variation with the .shy shaders. Ideally I think we'd want to be able to experiment with shaders and see the results on screen and reuse them more directly than is possible with the custom .shy shaders.

The examples are both about testing the VSG and illustrating it to new users, for those new users we want to minimize the number of new ideas that they have to learn as they will already be over-burdened with learning Vulkan, the VukanSceneGraph, GLSL etc, the more we can provide examples directly within these APIs/languages the easier it will be form them, and easier for us trying to guide new users through this process.

I don't have time right now to rewrite vsgshadertoy so will have to leave merging till I have a chance to properly tackle the changes I have in mind. Thanks for your patience.

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.

3 participants