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

Remove glsl-unit #1079

Merged
merged 6 commits into from
Oct 2, 2013
Merged

Remove glsl-unit #1079

merged 6 commits into from
Oct 2, 2013

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Oct 1, 2013

Unfortunately, the glsl-unit project (a GLSL minifier and templating system) seems to be dead. There are a number of outstanding bugs, the project has not been touched for a year and a half, and the original developer has moved on. The generated output is incompatible with IE11 (see #1051).

This PR adds a simple drop-in replacement for glsl-unit. It preserves the templating system (useful) and does simple minification, which works as well as GLSL unit for our simple shaders.

@twpayne
Copy link
Contributor Author

twpayne commented Oct 1, 2013

Note that with this PR it's possible to remove the generated *shader.js files from the source tree, since they can be rebuilt with requiring the user to install glsl-unit. I propose to do this in a separate PR.

@elemoine
Copy link
Member

elemoine commented Oct 1, 2013

Why not forking glsl-unit or writing a simple node-based tool replacement? It would be more inline with our will to use Node for the build system.

@twpayne
Copy link
Contributor Author

twpayne commented Oct 1, 2013

Why not forking glsl-unit or writing a simple node-based tool replacement?

Modifying glsl-unit to fix the outstanding bugs requires a moderate understanding of compiler technology. glsl-unit doesn't even come with build instructions.

I'm not familiar with node, and we have a dependency on Python for gjslint anyway.

@fredj
Copy link
Member

fredj commented Oct 2, 2013

See also https://npmjs.org/package/glslmin
The project has not been touched for 10 months and I don't know if it fits our needs.

I'm +1 for the python replacement

@twpayne
Copy link
Contributor Author

twpayne commented Oct 2, 2013

Thanks for the pointer to glslmin. It doesn't seem to support templating, and templating is useful to avoid having to write boilerplate code.

@elemoine
Copy link
Member

elemoine commented Oct 2, 2013

I don't like that we have both package.json and requirements.txt. This really demonstrates that we're going in any direction for the build system. I hope to find time to rewrite this in js.

Please merge.

twpayne added a commit that referenced this pull request Oct 2, 2013
@twpayne twpayne merged commit 57b4606 into openlayers:master Oct 2, 2013
@twpayne twpayne deleted the no-glsl-unit branch October 2, 2013 10:23
@twpayne
Copy link
Contributor Author

twpayne commented Oct 2, 2013

I've updated the developer guide to mention requirements.txt.

@twpayne twpayne mentioned this pull request Oct 2, 2013
@tschaub
Copy link
Member

tschaub commented Oct 3, 2013

It looks like it should be possible to use glsl-unit with goog.node.FLAGS.consolidate_declarations set to OFF.

@tschaub
Copy link
Member

tschaub commented Oct 3, 2013

This really demonstrates that we're going in any direction for the build system.

Agreed that the current lack of consensus on direction forward sucks.

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.

4 participants