-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Make Cesium's shaders editable with the SpectorJS shader editor #8608
Conversation
Thanks for the pull request @kring!
Reviewers, don't forget to make sure that:
|
By the way, |
Source/Renderer/ShaderProgram.js
Outdated
var vertexShaderText = options.vertexShaderText; | ||
var fragmentShaderText = options.fragmentShaderText; | ||
|
||
if (window.spector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we don't do a good job of it everywhere, in general this should be typeof spector !== 'undefined'
instead using window
, since window
doesn't exist in all contexts (workers/node/etc..) it's a more robust check for a global spector
variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Thanks again for your contribution @kring! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
Works perfectly for me. I confirmed that shader changes work and errors get propagated correctly. |
Live, on-the-fly editing of shaders. Next best thing to an actual shader debugger.
I hacked up CesiumJS to make this work while I was working on the log depth stuff, and found it super useful. So I thought I'd clean it up slightly and make it an official feature.
I considered wrapping the SpectorJS-code in
pragmas.debug
, but it's pretty low impact so I decided against it in the end.Fixes #5279