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

Implemented local shader arrays #30500

Merged
merged 1 commit into from
Jul 15, 2019
Merged

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jul 10, 2019

Arrays are missed as described in #10751. This PR is partly fixing it by implementing arrays support inside functions.

I do not touch the uniform arrays since they are required to change the visual server (and now its under heavy development by @reduz). But the provided feature is also good - for example its allows easy implement the Voronoi noise on https://thebookofshaders.com/12/

image

So about tech details..

The arrays can be any type(except sampler) and must have at least 1 element.

image

The accessing of the array using constant integer instruction. I've added static guard code to prevent user entering invalid indexes:

image

But If you pass an invalid index indirectly (by calling function inside or using variable) the rendering pipeline(not engine) will crash(I dont found a good way to solve this problem - maybe new render_mode is required to prevent this - and the shader code will suffering by using "if" statements - I prefer leave this untouched for now

If you need to get the size of the array - it provides the length function.

image

@Chaosus Chaosus requested a review from reduz as a code owner July 10, 2019 18:29
@Chaosus Chaosus added this to the 3.2 milestone Jul 10, 2019
@Chaosus Chaosus changed the title Implemented local shader arrays [WIP] Implemented local shader arrays Jul 10, 2019
@Chaosus Chaosus force-pushed the shader_arrays branch 2 times, most recently from a58a79f to b0a2fa0 Compare July 11, 2019 07:22
@Chaosus Chaosus changed the title [WIP] Implemented local shader arrays Implemented local shader arrays Jul 11, 2019
@Chaosus
Copy link
Member Author

Chaosus commented Jul 11, 2019

Ok, feel free to check @akien-mga, all build warnings are eliminated. The only missing feature for local arrays is ability to pass them into functions - I temporary disable this feature and added warning stub since its pretty boring to implement for me right now.. maybe later in another PR

@Chaosus
Copy link
Member Author

Chaosus commented Jul 12, 2019

@clayjohn Can you check and approve this please ?

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 so far. I don't have a lot of knowledge about the shader compiler stuff, but from what I can tell it looks fine. I tested out this branch pretty extensively and it seems to work great.

A few notes for documentation:

  • No multidimensional arrays
  • No array uniforms
  • constants cant be arrays
  • cant be initialized using array constructor syntax (e.g. float[5](1.0, 1.0, 1.0, 1.0, 1.0))
  • cant be used as parameters in functions (yet)
  • shader crashes if you read outside array length

@Chaosus
Copy link
Member Author

Chaosus commented Jul 12, 2019

Thanks @clayjohn ! Some thoughts about:

No multidimensional arrays

They are not supported in GLES3 - only GL4 feature set can support them - so they cannot be implemented (until Vulkan is come)

No array uniforms

Yeah, as mentioned above - I dont want to interrupt great work of @reduz

cant be initialized using array constructor syntax (e.g. float[5](1.0, 1.0, 1.0, 1.0, 1.0))

constants cant be arrays

Ah, yeah, I miss it..

shader crashes if you read outside array length

Yeah, its not simple to fix problem - if simply push internal "if" instruction to check array bounds the performance will be greatly suffering

@clayjohn
Copy link
Member

@Chaosus I wasn't being critical, those are notes for when we document the new feature. :) They are just important things that users will need to be aware of.

Take a look at https://www.khronos.org/opengl/wiki/Data_Type_(GLSL)#Array_constructors it describes the default array constructor.

@Chaosus
Copy link
Member Author

Chaosus commented Jul 12, 2019

I wasn't being critical, those are notes for when we document the new feature. :)

@clayjohn Nvm.. sorry sometimes things are slip away from my mind :)

@Calinou
Copy link
Member

Calinou commented Jul 12, 2019

Yeah, its not simple to fix problem - if simply push internal "if" instruction to check array bounds the performance will be greatly suffering

Maybe this could be done in editor/debug builds only?

@Chaosus
Copy link
Member Author

Chaosus commented Jul 12, 2019

Probably, someone should ask @reduz about adding this to 3.2 - I dont want to work on the PR if its state is undefined.

@akien-mga
Copy link
Member

As discussed with @reduz, this should be fine for 3.2, and will likely need to be partly reimplemented for Vulkan. Testing is needed on a wide range of hardware to make sure that those arrays don't run into hardware/driver limitations.

@akien-mga akien-mga merged commit e6230a3 into godotengine:master Jul 15, 2019
@akien-mga
Copy link
Member

Thanks!

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.

4 participants