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

webrender should probably include the shader source directly and not read it from the filesystem #352

Closed
jrmuizel opened this issue Aug 18, 2016 · 11 comments

Comments

@jrmuizel
Copy link
Contributor

commented Aug 18, 2016

This will make distribution easier and avoids these being user modifiable.

@metajack

This comment has been minimized.

Copy link

commented Aug 18, 2016

Long term I think this is the plan. But right now this is needed to be able to develop with a reasonable workflow since changing a compiled in shader would require a full servo rebuild.

@metajack

This comment has been minimized.

Copy link

commented Aug 18, 2016

cc @glennw

@jrmuizel

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2016

Doesn't webrender_traits prevent needing to do a servo rebuild?

@metajack

This comment has been minimized.

Copy link

commented Aug 18, 2016

webrender_traits prevents a serialization of the dependency tree, allowing more stuff to compile in parallel. You'll still have to rebuild any crates that depend (transitively) on webrender. This probably means you can skip the script crate build, but not the servo crate one (including the final link).

This is a lot of hassle compared to instantaneous shader reload with the file system watcher while running.

@glennw

This comment has been minimized.

Copy link
Member

commented Aug 18, 2016

It does, but it's still much slower to build and link WR than it is to tweak shaders unfortunately. We do need a solution though that embeds the shaders for distribution.

@metajack

This comment has been minimized.

Copy link

commented Aug 18, 2016

I assume we'll switch to include_str! at some point, although it would probably be best if there was a cargo feature flag to compile them in or leave them on disk.

@glennw

This comment has been minimized.

Copy link
Member

commented Oct 21, 2016

I think the plan here would be to have a build step that includes the shader source in WR. By default, WR would use these, however it would also look in a particular location (probably ../webrender/webrender/res/ from the servo directory) and load these as overrides if they are present. This would mean we don't need to copy the shaders to deploy, but still get the benefit of fast iteration time when developing.

@vvuk

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2016

I may do this real quick -- it just bit me (replace'd wr with local version, WR since servo's lock had modified shaders but no version bump), and would make updating WR easier in servo. I'll likely do this with a build.rs step that will collect all glsl files res and generate a .rs file that puts them all in a hash by name.

@staktrace

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2016

FWIW doing this now would be really timely since it's blocking having "runnable" quantum-render builds in automation. (i.e. running these QR builds fails because it can't find the res directory). I can work around it in Firefox build packaging but it would be nicer to not have to.

@vvuk vvuk self-assigned this Nov 16, 2016
@MSleepyPanda

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2016

@vvuk Just curious, why not use include_bytes or include_str instead of a build script which generates source code?

bors-servo added a commit that referenced this issue Nov 17, 2016
compile in shaders to webrender instead of requiring a res directory

Fixes issue #352.

res directory can be used to override.

<!-- 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/563)
<!-- Reviewable:end -->
@glennw

This comment has been minimized.

Copy link
Member

commented Nov 17, 2016

@glennw glennw closed this Nov 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.