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 ShaderLab language #3490

Merged
merged 4 commits into from Apr 20, 2017

Conversation

Projects
None yet
4 participants
@tgjones
Copy link
Contributor

tgjones commented Feb 22, 2017

This PR adds a new language definition for ShaderLab (used to write shaders in the Unity game engine). It uses the grammar that was already added as part of #3469.

I've moved the .shader extension from the GLSL language definition (where it was incorrect, but better than nothing) to the new ShaderLab language definition.

ShaderLab search result on GitHub.com

@larsbrinkhoff

This comment has been minimized.

Copy link
Collaborator

larsbrinkhoff commented Feb 23, 2017

.shader is a very generic file extension. I'm not a shader language expert; are really all those search result ShaderLab files?

@larsbrinkhoff

This comment has been minimized.

Copy link
Collaborator

larsbrinkhoff commented Feb 23, 2017

I believe these keywords are present in ShaderLab but unlikely to appear in other shader languages: ColorMask, CGINCLUDE, CGPROGRAM, _MainTex, SubShader. You're right, the vast majority of all .shader files is ShaderLab.

However, some of them are GLSL.

@tgjones

This comment has been minimized.

Copy link
Contributor

tgjones commented Feb 23, 2017

Ah, thanks for noticing that. Presumably I should add the .shader extension back to GLSL - but do I need to do anything, beyond providing samples, to help the classifier?

On a possibly related note, I think there's a bug in this classifier test:

Samples.each do |sample|
  language  = Linguist::Language.find_by_name(sample[:language])
  languages = Language.find_by_filename(sample[:path]).map(&:name)
  next unless languages.length > 1

  results = Classifier.classify(Samples.cache, File.read(sample[:path]), languages)
  assert_equal language.name, results.first[0], "#{sample[:path]}\n#{results.inspect}"
end

It never gets beyond the next unless..., because Language.find_by_filename is too specific, and only ever returns a single language. Language.find_by_extension seems to work better there, but I don't know the code well enough to know if that's the correct fix.

@pchaigno

This comment has been minimized.

Copy link
Collaborator

pchaigno commented Feb 23, 2017

It never gets beyond the next unless..., because Language.find_by_filename is too specific, and only ever returns a single language. Language.find_by_extension seems to work better there, but I don't know the code well enough to know if that's the correct fix.

Nice catch! That is the correct fix :-)
I missed this test when I changed find_by_filename in #2006 :/
Could you open a separate pull request to fix it?

@larsbrinkhoff

This comment has been minimized.

Copy link
Collaborator

larsbrinkhoff commented Feb 23, 2017

do I need to do anything, beyond providing samples, to help the classifier?

If the samples doesn't do the trick, lib/linguist/heuristics.rb is your friend.

@tgjones

This comment has been minimized.

Copy link
Contributor

tgjones commented Feb 23, 2017

Could you open a separate pull request to fix it?

I've opened #3494 with a fix for the ambiguous classification test.

If the samples doesn't do the trick, lib/linguist/heuristics.rb is your friend.

It looks like the samples are enough - assuming I fixed the test correctly in #3494, and that I'm testing the right thing :)

@tgjones

This comment has been minimized.

Copy link
Contributor

tgjones commented Mar 14, 2017

Please let me know if you need anything more from me on this PR.

@larsbrinkhoff

This comment has been minimized.

Copy link
Collaborator

larsbrinkhoff commented Mar 14, 2017

Travis says the pull request doesn't pass build/test.

Add sample GLSL .shader files
Note that these are copies of existing GLSL samples, renamed to have
the .shader extension.
@tgjones

This comment has been minimized.

Copy link
Contributor

tgjones commented Apr 9, 2017

Oops, somehow didn't notice that (or rather I did, but I looked at the build failure for a different Ruby version that had unrelated errors, and thought it wasn't caused by my changes).

Anyway, fixed now.

@larsbrinkhoff
Copy link
Collaborator

larsbrinkhoff left a comment

Looks good to me.

@lildude lildude merged commit dd53fa1 into github:master Apr 20, 2017

2 checks passed

GitHub CLA @tgjones has accepted the GitHub Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Alhadis added a commit to file-icons/atom that referenced this pull request Apr 21, 2017

Update config.cson with recent Linguist additions
Added:
* .pyi to Python extensions:       github/linguist#3548
* .shader to 3D object extensions: github/linguist#3490
* .mdwn to Markdown extensions:    github/linguist#3534
* Jest icon to .js.snap files:     github/linguist#3579
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment