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 GlslProg::create() method for transform feedback shaders. #1241

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paulhoux
Copy link
Collaborator

This allows you to quickly create the transform feedback shader by just supplying a file, a list of varyings and optionally a transform feedback format. Attributes don't need to be specified, because GlslProg already is able to query them from the source files. Thanks to this addition, I was able to add support for transform feedback shaders to a caching system, which would otherwise have been harder.

This allows you to quickly create the transform feedback shader by just supplying a file, a list of varyings and optionally a transform feedback format. Attributes don't need to be specified, because GlslProg already is able to query them from the source files. Thanks to this addition, I was able to add support for transform feedback shaders to a caching system, which would otherwise have been harder.
@richardeakin
Copy link
Collaborator

Sounds reasonable to add TF-oriented constructor to me, if nothing else than to make it clear to first time users of the technique what is needed. Maybe at least one of the samples should make use of this new create() method?

I wish we'd document the constructors, especially when they have arguments with defaults because some lesser IDEs (cough, cough, Xcode) tend to not even show those variables during auto-completion, but at the same time I realize the other ones aren't documented, either.

@paulhoux
Copy link
Collaborator Author

Yeah, it's a good idea to use it in a sample. Will add it to this PR tomorrow.

@ryanbartley
Copy link
Contributor

I agree. I also think there should be a constructor that takes a geometry shader as well.

@richardeakin
Copy link
Collaborator

@ryanbartley well that one becomes a slippery slope - then do we need a constructor for tesselation shaders too? compute? TF is sort of unique because it is commonly its own standalone vert shader.

@richardeakin
Copy link
Collaborator

So, there actually are GlslProg::create() methods that can take geometry shaders, I hadn't realized it because they default to empty.

It also got me thinking, what with the changes @paulhoux is proposing here, we could remove the default-empty fragment shader in these constructors, since AFAIK the only time you would use a vert-shader-only GlslProg would be in the case of transform feedback.

@cinder
Copy link
Owner

cinder commented Dec 27, 2015

My own opinion is that for just about every case that's not just vert+frag, GlslProg::Format should be used. In my view, tessellation shaders aren't such a common use case that the ratio of saved typing to complication of the class makes sense here, though it seems others feel differently?

@paulhoux
Copy link
Collaborator Author

paulhoux commented Jan 4, 2016

@andrewfb : there is a slight issue with gl::GlslProg::Format that is important in this discussion, and that is that all parsing is done during construction of the format. This of course includes file access. In those cases where you'd like to delay this, for example if you want to do this asynchronously, you need to have a way to specify the source files, as well as all other parameters (e.g. attributes), without using the format. In my opinion, this slightly defeats the purpose of the format as a descriptor of your shader and it's what made me add the new create method(s). In hindsight, though, I could probably have constructed a Format on the secondary thread and used it as a parameter instead. But I still think the format should be more like a descriptor, instead of doing the hard parsing work.

@richardeakin
Copy link
Collaborator

@paulhoux you can load the glsl into std::string's async and then construct the entire format right when you're create()ing the GlslProg. But yea, I don't see why you couldn't create the entire Format on a background thread and then copy (move if you prefer) to the main thread to load. It's actually nice that the file i/o is happening at the Format for this reason come to think of it, because it allows you to at least handle that part async if you wish. Tempting to move the shader preprocessing there as well..

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