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

Add support for shader includes. #1588

Merged
merged 2 commits into from Aug 21, 2017
Merged

Add support for shader includes. #1588

merged 2 commits into from Aug 21, 2017

Conversation

@glennw
Copy link
Member

glennw commented Aug 18, 2017

If the first line of a shader is //import followed by a comma
separated list of glsl filenames, prepend those as includes
for the shader. The idea here is that we'll start splitting
up prim_shared.glsl etc into smaller blocks that are faster
to parse and compile, and just generally more manageable.

This also supports having the VS and FS in the same glsl file. This
commit ports the debug_font and debug_color shaders to this style.

If we go ahead with this format, we'll incrementally port other
shaders to use this style, which requires only one file per shader.


This change is Reviewable

@glennw
Copy link
Member Author

glennw commented Aug 18, 2017

r? @kvark and @nical

It's always a bit of an open question how to manage shader includes and common code with GLSL. This seems to work reasonably well, and would allow us to modularize our shared GLSL code a lot. What do you think?

@nical
nical approved these changes Aug 18, 2017
Copy link
Collaborator

nical left a comment

I like the idea. I am not super found of hard-coding that imports need to be on the first line (if you don't know it's easy to trip on this, and the imports kinda get hidden behind the license block). but I suppose we can improve this later if turns out to be a significant annoyance.

const SHADER_KIND_VERTEX: &str = "#define WR_VERTEX_SHADER\n";
const SHADER_KIND_FRAGMENT: &str = "#define WR_FRAGMENT_SHADER\n";
const SHADER_IMPORT: &str = "//import ";
const SHADER_LINE_MARKER: &str = "#line 1\n";

This comment has been minimized.

@nical

nical Aug 18, 2017

Collaborator

Neat!

@kvark
kvark approved these changes Aug 18, 2017
Copy link
Member

kvark left a comment

This is very similar to what I ended up using in three-rs:

#version 150 core
#include locals lights
...

Except for:

  • I allow includes anywhere. Sounds like @nical agrees with it.
  • I use # directive instead of //. I don't think it makes sense to avoid parsing the lines by no-WR parsers since any sort of compilation would break anyway.

@glennw would you agree to make these two improvements in this PR?

let mut source = Vec::new();
source.extend_from_slice(s.as_bytes());
gl.shader_source(id, &[&source[..]]);
let sources: Vec<&[u8]> = sources.iter().map(|s| s.as_bytes()).collect();

This comment has been minimized.

@kvark

kvark Aug 18, 2017

Member

I'd prefer us building a single source chunk per shader (as opposed to having an array of chunks), because OpenGL is the only API supporting multiple sources.

let mut shared_strings = Vec::new();
if let Some(shared_source) = get_shader_source(base_filename, &self.resource_override_path) {
parse_shader_source(shared_source,
&self.resource_override_path,

This comment has been minimized.

@kvark

kvark Aug 18, 2017

Member

time to learn the new formatting style ;)

gw3583 added 2 commits Aug 18, 2017
If the first line of a shader is //import followed by a comma
separated list of glsl filenames, prepend those as includes
for the shader. The idea here is that we'll start splitting
up prim_shared.glsl etc into smaller blocks that are faster
to parse and compile, and just generally more manageable.

This also supports having the VS and FS in the same glsl file. This
commit ports the debug_font and debug_color shaders to this style.

If we go ahead with this format, we'll incrementally port other
shaders to use this style, which requires only one file per shader.
* Use #include.
* Support includes on any line.
* Update validation test to be closer to what we actually compile.
@glennw glennw force-pushed the glennw:shader-compiler branch from c01130b to b19aabe Aug 20, 2017
@glennw
Copy link
Member Author

glennw commented Aug 20, 2017

OK, updated to use #include, support includes on any line, use a single source string, and also updated the shader validation test to be more representative of what we actually compile (and use the same shared code to build the shader string).

@kvark
Copy link
Member

kvark commented Aug 21, 2017

Thank you!
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2017

📌 Commit b19aabe has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2017

Testing commit b19aabe with merge 1b87700...

bors-servo added a commit that referenced this pull request Aug 21, 2017
Add support for shader includes.

If the first line of a shader is //import followed by a comma
separated list of glsl filenames, prepend those as includes
for the shader. The idea here is that we'll start splitting
up prim_shared.glsl etc into smaller blocks that are faster
to parse and compile, and just generally more manageable.

This also supports having the VS and FS in the same glsl file. This
commit ports the debug_font and debug_color shaders to this style.

If we go ahead with this format, we'll incrementally port other
shaders to use this style, which requires only one file per shader.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1588)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2017

☀️ Test successful - status-travis
Approved by: kvark
Pushing 1b87700 to master...

@bors-servo bors-servo merged commit b19aabe into servo:master Aug 21, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.