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

Webpack glslify #270

Closed
wants to merge 2 commits into from
Closed

Webpack glslify #270

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 11, 2015

No description provided.

fragment: './FragmentShader.glsl'
});
var shaders = {
vertex: glslify('./VertexShader.glsl'),
Copy link
Member

Choose a reason for hiding this comment

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

spacing

Copy link
Member

Choose a reason for hiding this comment

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

@adnan-wahab can you please fix the spacing?

@MylesBorins
Copy link

lgtm :D
This should allow for basic support in other loaders for now

@michaelobriena
Copy link
Member

@adnan-wahab Are the changes to the shader meant to be included as well?

@alexanderGugel
Copy link
Member

I don't really see how this addresses the issue. While this now supports Webpack, we still rely on the glslify transform, which means we rely on glslify-loader. E.g. how does this address usage via AMD?

I thought we were moving away from relying on glslify in library code (and instead moving it into a bundler agnostic build step, like in the previous PR).

@MylesBorins
Copy link

If someone is using another module loading paradigm such as AMD they will still be able to use the built version of famous without a problem.

This will at least fix support with webpack and I believe jspm. It is enough for us to support our community while we figure out the most graceful way to EOL glslify

@alexanderGugel
Copy link
Member

@thealphanerd I'm not sure I understand how this is going to support AMD without usage of the global build. The current solution would still require a preprocessing step for those people from my understanding.

@MylesBorins
Copy link

People using amd are going to require a preprocessor either way was my
point.
On Jun 11, 2015 4:43 PM, "Alexander Gugel" notifications@github.com wrote:

@thealphanerd https://github.com/TheAlphaNerd I'm not sure I understand
how this is going to support AMD without usage of the global build. The
current solution would still require a preprocessing step for those people
from my understanding.


Reply to this email directly or view it on GitHub
#270 (comment).

@alexanderGugel
Copy link
Member

Hmmmm.... ok. I don't really see why we can't just put the shader in the repo to be honest (other than rather purist reasons), but maybe I'm missing something. Anyways. If that solves the issue...

@ghost
Copy link
Author

ghost commented Jun 12, 2015

@michaelobriena
yes the changes to the shader are required for this to work

@alexanderGugel
Copy link
Member

@adnan-wahab Is it because glslify renames the uniforms for some reason? I had that issue, but thought I might be missing something. Still seems kind of a glslify bug to me. If that's the case, we should probably fix that in glslify...

@jd-carroll
Copy link
Contributor

+1 for putting it directly in the repo and dumping glslify

Not sure if that's even possible, but it would be great to use grunt/gulp + bower for build and dep management. Right now I find it near impossible to incorporate that type of build because of the glslify. You are left in a state where some dep's are managed through bower and others are through node 😬.

I could be completely wrong, but I would love to be able to list famous as a bower dep and then use traditional gulp / grunt.

@michaelobriena
Copy link
Member

@jd-carroll We could still publish to bower with the GLSL code built already. This would allow people to make changes to our shader code and use those changes without having to download a different tool to compile the shaders for them.

@jd-carroll
Copy link
Contributor

I think the bower + grunt/gulp use case is very important. Without it, I
think adoption rates will be lower than what they otherwise could be.

On Sun, Jun 14, 2015 at 6:56 PM, michaelobriena notifications@github.com
wrote:

@jd-carroll https://github.com/jd-carroll We could still publish to
bower with the GLSL code built already. This would allow people to make
changes to our shader code and use those changes without having to download
a different tool to compile the shaders for them.


Reply to this email directly or view it on GitHub
#270 (comment).

@MylesBorins
Copy link

I very much disagree.

We can easily support bower simply by including the standalone in our
tagged version of the platforms

Aside from this the tooling /module loading pattern of the use is a moot
point.

If a developer has a preference, there is nothing stopping them from sticking
famous in their pipeline. Whether it be a script tag or via require (the
standalone will support amd)

If a develop has no preference or does not have experience they can use our
supported work flow with the famous cli and follow along our documentation
to get started.

If you consider that every in production app will have very different
pipeline needs, we cannot ever hope to offer example projects using every
potential tooling system.

What we cam do, and will, is make sure there is nothing in our build that
blocks our developers from using their tool chain of choice.
On Jun 14, 2015 7:47 PM, "Joseph Carroll" notifications@github.com wrote:

I think the bower + grunt/gulp use case is very important. Without it, I
think adoption rates will be lower than what they otherwise could be.

On Sun, Jun 14, 2015 at 6:56 PM, michaelobriena notifications@github.com
wrote:

@jd-carroll https://github.com/jd-carroll We could still publish to
bower with the GLSL code built already. This would allow people to make
changes to our shader code and use those changes without having to
download
a different tool to compile the shaders for them.


Reply to this email directly or view it on GitHub
#270 (comment).


Reply to this email directly or view it on GitHub
#270 (comment).

@alexanderGugel
Copy link
Member

@thealphanerd I agree from a conceptual point of view, but I think the truth is that we can easily support a wider range of tool chains by simply checking in the shader code. This seems like a very acceptable cost considering the increase in usability and that our dependency on glslify has been a problem in the past. I think that's a much more flexible solution than forcing people to use the standalone version.

We can easily support bower simply by including the standalone in our tagged version of the platforms

If we do this, I see no reason why we shouldn't also include the bundled shader code.

Aside from this the tooling /module loading pattern of the use is a moot point.

There have been numerous complains about our dependency on browserify and glslify. IMO we should aim for making less assumptions about the tooling used by the user. Removing the glslify dependency for using the platform is a step in the right direction IMO.

@MylesBorins
Copy link

@alexanderGugel while I agree with your points, I respectfully disagree in practice.

Our code base makes a few assumptions

  • Common JS module loading pattern
  • Browserify build step

This stack allows for us to do some magical things in development, like what we are doing with glslify. It will also support us being able to do something fancy for assertion like you are suggesting in #285

We need to make a higher level decision if we want to rely on AST transformation between development and production. If so, there is no need to check in the GL code. If people want to use our "development" code in production, the only thing stopping them from doing so is their technology stack. If someone chooses to use webpack, then they can either require in our standalone (which will just work), or they can shim the variety of AST manipulations we want to use.

There is no disadvantage to using the standalone build, especially as we can include a version that is unminified and includes source maps. There is nothing stopping them from using the development version either, as they can easily rebuild in-between steps.

The amount of build systems and tooling stacks has no shortage, webpack, JSPM, systemJS, bower, npm, commonJS, AMD, es6.... etc etc etc

We simply cannot support all the edge cases in our development branch, but we can very easily publish a version that is compatible.

If we decide that we need our development branch to be a baseline that works in all systems we are effectively giving up the ability to do AST transformations as part of our development process. Perhaps that is preferred... but I think it is super useful and we should be careful about throwing it away

@alexanderGugel
Copy link
Member

Our code base makes a few assumptions

  • Common JS module loading pattern
  • Browserify build step

The browserify build step is what I'm worried about. One of the advantages of #285 IMO is that everything also "just works fine" without the transform.

We simply cannot support all the edge cases in our development branch, but we can very easily publish a version that is compatible.

I completely agree! Relying on glslify by contrast means the user needs to have Node installed when creating custom builds.

I think checking in the generated glslify is really not a big deal, especially if it increases usability in some cases. That being said I don't have an extremely strong opinion about that since - as you said - users can always still use the standalone build when in doubt. The only thing that I'm worried about is that we need to special case everything because of glslify (e.g. glslify-loader for Webpack), which is IMO not needed.

@michaelobriena
Copy link
Member

@alexanderGugel I think our main goal is to support multiple build tools and this does that. I think moving away from a strict dependency on browserify is a good goal to have but having glslify is great for engine development (which is the point of this repo).

If you don't want our tooling, you can use other tools (because of this change) or just use the standalone as Myles mentioned. I don't think it should be the job of our engineers to maintain personal glsl transform tools just because we want to move off of glslify for no real benefit.

@michaelobriena michaelobriena added this to the 0.6.0 milestone Jun 16, 2015
@michaelobriena
Copy link
Member

landed as e4e0f0f

@talves
Copy link

talves commented Jun 16, 2015

As a note regarding gulp and browserify, I have a working famous-gulp-seed which with initial testing supports browserify and the glslify dependency.

  • Compatible with the famous-cli commands and structure using public also.
  • Allows for multiple bundle files (multiple entry points)

What I personally think would be a great add for the famous-cli is to allow for the passing of your own seed path on famous create by adding a prompt for a seed. The default would be the famous seed. People with little tooling experience can use a third party seed. Just an idea.

@trusktr
Copy link
Contributor

trusktr commented Jul 28, 2015

@adnan-wahab Do you have a working webpack example? If so, I'm curious to learn from it. If not, I'll have one soon for everyone. (:

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.

7 participants