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
HXSL: Implement borrow qualifier #916
Conversation
Note to @ncannasse : I didn't know |
hxsl/Linker.hx
Outdated
@@ -88,19 +110,19 @@ class Linker { | |||
} | |||
// add a new field | |||
if( ft == null ) | |||
fl2.push(allocVar(f1,p).v); | |||
fl2.push(allocVar(f1,p, null, null, shaderName).v); |
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.
I don't like much that shaderName seems to be requested to track all vars correctly, and yet it's an optional parameter at the end of parameters. Move it either as first optional parameter and eventually make it not-optional.
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.
I'll move it, but I can't make it non-optional, because the shader name information is not available in all cases where variables are allocated (i.e. inside functions) and borrowing supposed to work only for params anyway, hence its passed only when allocating the general variables and omitted for everything else.
Apart from my review in code, i'm fine with the feature. Might require some documentation https://github.com/HeapsIO/heaps/wiki/HXSL#variable-kind |
Simplify mergeVar borrow check Move shaderName argument.
Ping. Also regarding docs: I'll update it when PR's merged, so no worries. |
Thanks ! merged |
Implements
@borrow(path.to.Shader)
qualifier to variables.This is essentially reverse of the
@shared
qualifier and intended only for advanced usage. (As pretty much everything HXSL, frankly.) Lets user-defined shaders to borrow uniforms from other shaders. Does not perform checks for that shader actually being attached and is completely on the conscience of the user to ensure shaders they borrow from are present. This a follow up to #912 as an alternative way to access foreign uniforms.Usage example
In this shader I already posted before I need to know the texture size of Base2d to perform custom sampling filter transition curve that is specifically made for pixel-art. And "just set texture to what you going to render beforehand" is not a solution.
Reasoning
@param
, and so far only solution was to make a duplicate param, and ensure that you set it to same value as the one want data from. And this is clearly not a very good solution.@shared
" is no longer applicable, since borrowing is an explicit action from the user, and it doesn't matter if name is common or not. Especially since it requires to specify which shader it borrows from. I can make it as simple as just@borrow
but then it's not very clear what it borrows and from where.Extra notes
Because of how duplicate name is implemented, common names are a big problem in general for borrow and shared qualifiers, because those checks happen only for first variable, there are edge cases that can be resolved only by accounting for all variables that were renamed due to naming collision. Such as:
This wont compile btw, because
color3
is never assigned. Reverse is applicable as well, with@borrow(ShaderB) var color:Vec4;
This is obviously because ShaderB, which shares its uniform was processed after ShaderA, which occupied the
color
ID, causing it to be renamed intocolor2
, and by the time ShaderC get processed and expects the shared uniform from ShaderB - it gets thecolor
that is not marked as shared, causing it to be considered unique, disregarding thatcolor2
is shared.