-
-
Notifications
You must be signed in to change notification settings - Fork 21.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
Added a FULLRECT_UV keyword in canvas item #32861
Conversation
@QbieShay Can you add this input also to Visual Shaders? Otherwise, I will create a new PR for that afterwards. |
@Chaosus I'm not very familiar with visual shaders, so it'd probably have to wait a bit if I am the one to do it. If you know how to do it, I'd suggest that you add it :) |
Alright, I will keep track this PR then |
Im wondering what the benefit of having this built-in is over just letting users write:
|
@clayjohn that's the first thing i tried, but doesn't work. |
Sorry it has taken so long to review. I see why this change is needed. When atlas textures are used, both UV and VERTEX are transformed before reaching the user. So the user has no means to get the UVs of the rect itself. Thank you for your patience. @Chaosus please add this to the visual shader once merged. edit: @akien-mga please double check with reduz that he is okay adding a new built in |
@clayjohn no problem! |
Varyings are very expensive on mobile (they occupy tile sram) and drivers are not warranted to do intra stage optimization. Also given this is not something often used at all, I suggest doing the varying in your own shader instead.. |
@reduz there is no way for a user to do this in a shader because the UV coordinates that are received are already processed; however, I see your point with the lack of use cases. I'd leave this open in case someone else has this need, if that's okay. |
If this is too expensive on mobile, it could be put behind a project setting that's toggled automatically using feature tags. We should also measure the performance impact on desktop before enabling this by default. |
@Calinou we discussed this on devel earlier. Consensus was that we just properly expose it as a built-in to the vertex shader and then let users pass it as a varying themselves. That way there is zero overhead when people don't use the variable as it will be optimizes out by the compiler. |
88e29ad
to
20c5f53
Compare
Apologies, I haven't been the fastest but now I did as discussed :) |
@QbieShay I thought we settled on the name |
@clayjohn I named it this way because I thought it would be more intuitive for users. Why do you say it's not UV? Vertex is generally used for vertex position and not for uvs. That was my reasoning. |
The value of SRC_UV is just the initial untransformed vertex position. UV values are always between 0-1. With this ,SRC_UV can be any value. |
bc577c6
to
6238dff
Compare
@QbieShay Is this still desired? If so, it needs to be rebased on the latest master branch. |
@aaronfranke thank you for the reminder! I'll do this in the next days. |
@QbieShay It seems that you rebased incorrectly. Please read this article for more information, or if you prefer graphical Git clients, you can watch this video. |
@aaronfranke i rebased against 3.2, i have to wait to make the 4.0 PR until 2D is done.. |
There are some cases where having information on the position compared to the full quad is useful, for example a vertical fade based on the quad's y UV coordinate.
I don't think this will work with batching or nvidia workarounds, as is. |
Just to provide more info as Akien was asking about the PR: It seems to rely on the old method of drawing rects, which is now rarely used (as of 3.2.4). As well as working with the old method, it would need to be specifically written as an optional path for nvidia_workarounds, and have a dedicated path in the batching with a larger vertex format for this data, and as reduz says the extra varyings are not free, and then this would require testing phase. Also presumably it would have to be supported in vulkan if it were to done in 4.0. And all the ninepatch methods if it was desired there too. Overall it is a lot more involved than this PR suggests. Long term it could be worth it in some form, but it may be better to try and include it in a more thought out system for custom vertex data. |
I don't know the specifics of how rects are drawn in 3.2.4, but i trust your judgement on this. Just note that there is no added varying (there used to be), but the value is present only in the vertex shader. |
Practically speaking I think this would need to be filled out quite a bit more to work in 3.x. At a minimum I think there would need to be an implementation for the See e.g. It is doable but not as simple as the PR currently suggests, so closing. If you do want to fill it out to work more, we can work through this (either reopening this PR or a fresh PR), there should already be code in the compilers we use for detecting keywords and selecting, @clayjohn showed me this originally, I forgot some of the details as it is a long time since I worked on it. In addition I strongly suspect such an addition could be made more generic to support more use cases. I.e. instead of supporting just one extra thing in the vertex format, if you are going to add the machinery to do this, it seems more useful to make the data user definable, which makes it useful in a lot more scenarios. Also, see #54751 which exposes Also see godotengine/godot-proposals#3465 which is relevant. |
Workaround without batching: |
There are some cases where having information on the position compared
to the full quad is useful, for example a vertical fade based on the
quad's y UV coordinate.
Currently, cases such as atlas and 9patch don't bring information regarding the quad uv.
example below, the big white robot is a regular sprite while the smaller block on the
left is an atlas. note the different alpha.
with
COLOR.a = UV.y
with
COLOR.a = FULLRECT_UV.y
An alternative solution would be to pass unprocessed UV in the vertex shader, but it could break compat.
This does not work for GLES2's 9patch rect.
Artwork by Manuel Bustamante: https://www.artstation.com/artwork/3oa9lJ