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

Added a FULLRECT_UV keyword in canvas item #32861

Closed
wants to merge 1 commit into from

Conversation

QbieShay
Copy link
Contributor

@QbieShay QbieShay commented Oct 16, 2019

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
Screenshot_2019-10-16_11-12-15
with COLOR.a = FULLRECT_UV.y
Screenshot_2019-10-16_11-11-53

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

@Chaosus
Copy link
Member

Chaosus commented Oct 16, 2019

@QbieShay Can you add this input also to Visual Shaders? Otherwise, I will create a new PR for that afterwards.

@Chaosus Chaosus added this to the 3.2 milestone Oct 16, 2019
@QbieShay
Copy link
Contributor Author

@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 :)

@Chaosus
Copy link
Member

Chaosus commented Oct 16, 2019

Alright, I will keep track this PR then

@clayjohn
Copy link
Member

Im wondering what the benefit of having this built-in is over just letting users write:

varying vec2 full_uv;
void vertex () {
  full_uv = VERTEX.xy;
}

@QbieShay
Copy link
Contributor Author

@clayjohn that's the first thing i tried, but doesn't work.

@clayjohn
Copy link
Member

clayjohn commented Nov 3, 2019

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

@QbieShay
Copy link
Contributor Author

QbieShay commented Nov 3, 2019

@clayjohn no problem!

@reduz
Copy link
Member

reduz commented Nov 3, 2019

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..

@QbieShay
Copy link
Contributor Author

QbieShay commented Nov 3, 2019

@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.

@Calinou
Copy link
Member

Calinou commented Nov 18, 2019

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.

@clayjohn
Copy link
Member

@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.

@QbieShay
Copy link
Contributor Author

Apologies, I haven't been the fastest but now I did as discussed :)

@clayjohn
Copy link
Member

@QbieShay I thought we settled on the name SRC_VERTEX because the value isn't really a UV value. It is just the un-edited vertex value.

@QbieShay
Copy link
Contributor Author

QbieShay commented Nov 21, 2019

@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.

@clayjohn
Copy link
Member

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.

@QbieShay QbieShay force-pushed the fullrect_uv branch 2 times, most recently from bc577c6 to 6238dff Compare November 30, 2019 17:13
@aaronfranke
Copy link
Member

@QbieShay Is this still desired? If so, it needs to be rebased on the latest master branch.

@QbieShay
Copy link
Contributor Author

QbieShay commented Jul 1, 2020

@aaronfranke thank you for the reminder! I'll do this in the next days.

@QbieShay QbieShay requested a review from a team as a code owner October 25, 2020 13:45
@aaronfranke
Copy link
Member

@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.

@QbieShay QbieShay changed the base branch from master to 3.2 October 25, 2020 17:55
@QbieShay
Copy link
Contributor Author

@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.
@lawnjelly
Copy link
Member

I don't think this will work with batching or nvidia workarounds, as is.

@lawnjelly
Copy link
Member

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.

@QbieShay
Copy link
Contributor Author

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.

@QbieShay QbieShay requested a review from a team as a code owner March 12, 2021 12:26
Base automatically changed from 3.2 to 3.x March 16, 2021 11:11
@akien-mga akien-mga modified the milestones: 4.0, 3.4 Aug 11, 2021
@akien-mga akien-mga modified the milestones: 3.4, 3.5 Oct 24, 2021
@lawnjelly
Copy link
Member

lawnjelly commented Dec 2, 2021

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 nvidia_workaround path, which is now the default (to prevent glitches), and there would need to be selection code to detect this type of rect (use of the keyword in the shader) and prevent batching (so it uses a default command), adding a batched path for this would probably be overkill.

See e.g. rasterizer_canvas_gles2.cpp, line 418 to see the nvidia_workaround path, which calls _draw_gui_primitive which would need to be modified, and also made to work without sending the data in the vertex format when not in use.

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 gl_VertexID in GLES3 I believe, this may enabled the desired behaviour in a custom shader.

Also see godotengine/godot-proposals#3465 which is relevant.

@QbieShay
Copy link
Contributor Author

QbieShay commented Sep 4, 2022

Workaround without batching: vec2 full_uv = step(vec2(0.0), VERTEX.xy);
To be tested with batching.

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.

9 participants