Skip to content

Implement specialization constants #50325

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2021
Merged

Implement specialization constants #50325

merged 1 commit into from
Jul 11, 2021

Conversation

@reduz
Copy link
Member

@reduz reduz commented Jul 9, 2021

  • Added support to our local copy of SpirV Reflect (which does not support it).
  • Pass them on render or compute pipeline creation.
  • Not implemented in our shaders yet.

image

@reduz reduz requested review from a team as code owners July 9, 2021 20:29
@reduz reduz force-pushed the implement-specialization-constants branch from 6e0224d to 048b0ae Compare July 9, 2021 20:34
@reduz reduz mentioned this pull request Jul 9, 2021
Closed
@akien-mga akien-mga force-pushed the implement-specialization-constants branch from 048b0ae to ef3415c Compare July 9, 2021 20:55
@akien-mga akien-mga added this to the 4.0 milestone Jul 9, 2021
@akien-mga
Copy link
Member

Force pushed an amend to cleanup the thirdparty changes and add a patch file so they can be reapplied on updates.

clayjohn
clayjohn reviewed Jul 11, 2021
View reviewed changes
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good to me! I reviewed and nothing really jumped out. I haven't worked with specialization constants before so I can't catch much beyond simple errors.

@reduz @akien-mga
Implement Specialization Constants
b2f6db7 
* Added support to our local copy of SpirV Reflect (which does not support it).
* Pass them on render or compute pipeline creation.
* Not implemented in our shaders yet.
@akien-mga akien-mga force-pushed the implement-specialization-constants branch from ef3415c to b2f6db7 Compare July 11, 2021 21:16
@akien-mga akien-mga merged commit d1f25bb into godotengine:master Jul 11, 2021
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou mentioned this pull request Jul 12, 2021
Merged
@shangjiaxuan shangjiaxuan mentioned this pull request Jul 28, 2022
Open
@shangjiaxuan
Copy link

Any plans on putting this to upstream? I'm opening a pr upstream, and working on adding this to the main repo (with some added error checks, planning to add an evaluator that can evaluate some SpecConstantOp), and it seems khronos need committers to sign a license agreement. I was wondering if that's okay.

@shangjiaxuan
Copy link

Also note that the code in godot doesn't support 64 bit constants (they take up two words in instruction).

@akien-mga
Copy link
Member

@shangjiaxuan Thanks for working on upstreaming this. I think you can consider that the Godot patch is under the same license as SPIRV-Reflect, as if it had been PR'ed directly. @reduz can comment to confirm.

You could also add reduz as co-author by amending the commit message with: Co-authored-by: Juan Linietsky <reduzio@gmail.com>.

@reduz
Copy link
Member Author

reduz commented Jul 28, 2022

I confirm the license.

@akien-mga akien-mga mentioned this pull request May 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clayjohn clayjohn clayjohn left review comments

@akien-mga akien-mga akien-mga approved these changes

Assignees

No one assigned

Labels

enhancement topic:rendering topic:thirdparty

Projects

None yet

Milestone

4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@reduz @akien-mga @shangjiaxuan @clayjohn