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

[WIP] Add basic import shader support #41583

Closed
wants to merge 1 commit into from

Conversation

fire
Copy link
Member

@fire fire commented Aug 28, 2020

Implements godotengine/godot-proposals#944. Also see #17303. This salvages #17797.

The syntax is import "res://new_shader.shader";

image

@fire fire marked this pull request as draft August 28, 2020 17:42
@fire fire changed the title Add basic import shader support [WIP] Add basic import shader support Aug 28, 2020
@fire
Copy link
Member Author

fire commented Aug 28, 2020

Import Shader Project.zip

Example project.

Remaining problems:

  1. Caching is incorrect.
  2. Nested includes needs a better method.
  3. The highlighter sees "res://" as a comment rather as a string.
  4. implement as shader_type import;
  5. Add compiler errors.

Co-authored-by: bitsawer <sawerduster@gmail.com>
@fire
Copy link
Member Author

fire commented Aug 28, 2020

@clayjohn mentioned a proposal to make a new shader type import.

shader_type import;

<ClayJohn> [It] can only be included by other shaders. It wouldn't be allowed to have render modes or processor functions.
<ClayJohn> Includes also completely break error reporting. So we would need a more robust shader editor to track errors in included files.

Edited: Don't have much effort on today 2020-08-28. So I'll circle back to the topic at a later date.

Edited:

Changed include to import.

@Ansraer
Copy link
Contributor

Ansraer commented Sep 13, 2020

Since you have mentioned #17303 and #17797, do you have any intentions to add preprocessor directives to this PR?

@fire
Copy link
Member Author

fire commented Sep 13, 2020

No.

You can use uniforms and variables for #define and if else statements are already in the language.

@fire fire marked this pull request as ready for review November 12, 2020 03:50
@fire
Copy link
Member Author

fire commented Dec 23, 2020

This needs to be ported to the latest master. It is salvageable, but I am currently lacking effort on it.

The last tasks are to make sure the include detection handles multiple layers correctly.

@MagdielM
Copy link

I am interested in salvaging this. As of right now, I've managed to port the changes to c0d01dc and at least gotten around import not being recognized as a shader type. I'll admit I'm not too familiar with how the parser works as of yet, but I care enough about this feature to put in the time.

@CsloudX
Copy link

CsloudX commented Jul 15, 2021

I need this feature too.

@fire
Copy link
Member Author

fire commented Jul 15, 2021

@MagdielM did you post your work anywhere?

@MagdielM
Copy link

@MagdielM did you post your work anywhere?

Oh sorry, after three-or-so weeks of trying to get multiple nesting levels to work and not getting very far, other responsibilities started piling up and I had to put this in the backburner for the time being. I apologize for not mentioning this earlier.

@Calinou
Copy link
Member

Calinou commented Jul 22, 2021

Oh sorry, after three-or-so weeks of trying to get multiple nesting levels to work and not getting very far, other responsibilities started piling up and I had to put this in the backburner for the time being. I apologize for not mentioning this earlier.

Could you push your updated code to a branch in your fork and link it here? It would likely be helpful if someone is willing to continue this.

Also, I'd argue that even supporting a single level of #include is much better than supporting zero 🙂

@MagdielM
Copy link

My work includes a small dependency graph that is supposed to update all included shaders, but I couldn't quite figure out why the parser fails to nest the #includes, so ultimately it doesn't do much.
cd198c27a1e9bbdc4fa60d5ae07be3b88ff8f599

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.

None yet

6 participants