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 render_mode world_vertex_coords in canvas #24002

Closed

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Nov 27, 2018

This PR brings the render mode world_vertex_coords to canvas item shaders. It also provides access to two new uniform matrices for converting to and from world coordinates named GLOBAL_MATRIX and INV_GLOBAL_MATRIX respectively. I would prefer to name them WORLD_MATRIX (which is what it is internally) and INV_WORLD_MATRIX, however, for some reason the modelview transform is accessed using WORLD_MATRIX (which is incredibly confusing for users).

The behaviour is the same as in spatial shaders. It presents VERTEX to the user in world coordinates, and then transforms it into view and NDC coordinates after the user written code is run.

Many users have asked for access to this render mode and it directly addresses issue #19800. With the current system there is no way to acquire world position in the shader without passing in the global transform as a mat4 (which is currently broken, but fixed with #23976) or passing in the global position, scale, rotation and calculating the position manually (which is needlessly complicated/difficult).

Bugsquad edit: Fixes #19800.

@Chaosus
Copy link
Member

Chaosus commented Dec 10, 2018

Moving to 3.2 milestone as we are reaching beta stage for 3.1 and no longer merging new features.

@Chaosus Chaosus modified the milestones: 3.1, 3.2 Dec 10, 2018
@akien-mga
Copy link
Member

Actually I'd like @reduz to have a glance at this one before moving to 3.2. It doesn't really impact the existing features, so it could still be merged.

@akien-mga akien-mga modified the milestones: 3.2, 3.1 Dec 10, 2018
@reduz
Copy link
Member

reduz commented Dec 11, 2018

I am not sure about this for a number of reasons:
-Goes against eventually adding batching (so maybe, you could check if this is used, and if not the geometry is batched)
-Will likely hit performance a tiny bit on mobile, though probably not so bad.

Would kick for after 3.1 is out to discuss properly.

@akien-mga akien-mga modified the milestones: 3.1, 3.2 Dec 11, 2018
@clayjohn
Copy link
Member Author

@reduz do you think it would be a good idea to make a note in the documentation for now?

@akien-mga
Copy link
Member

Might be time to revisit this PR?

@clayjohn
Copy link
Member Author

Sure! I can resolve conflicts and we can see what reduz thinks.

@akien-mga
Copy link
Member

Bump :)

@clayjohn clayjohn force-pushed the add_canvas_shader_world branch from 56a2244 to fbefd13 Compare July 23, 2019 20:21
@clayjohn
Copy link
Member Author

@akien-mga Thanks for the reminder :)

The merge conflict led me to realize that there was a bug in nearby code that would have broken particles in GLES2 if uses used "skip_vertex_transform". So I fixed the bug and included it in this PR. If reduz decides not to merge this for 3.2 then I need to fix the bug in its own PR.

@clayjohn clayjohn force-pushed the add_canvas_shader_world branch from fbefd13 to 337c43b Compare July 23, 2019 20:25
@clayjohn clayjohn force-pushed the add_canvas_shader_world branch from 337c43b to 88f1c27 Compare July 24, 2019 19:24
@akien-mga
Copy link
Member

We discussed this on IRC, @reduz is not so keen on merging this as it has already been fixed properly in the vulkan branch for 4.0, and he prefers not to add a workaround in 3.2 in the meantime:

13:56 <Akien> #24002 (clayjohn)
13:56 <IssueBot> #24002: Added render_mode `world_vertex_coords` in canvas | https://git.io/fpzJP
13:57 <Akien> This one was "Would kick for after 3.1 is out to discuss properly." but the discussion didn't really happen :D
13:57 <reduz> I think I changed this in 4.0 branch
13:57 <reduz> or actually yes, its different in 4.0, so you dont need this
13:58 <Akien> Would it be worth merging for 3.2 only?
13:59 <reduz> i wouldnt change 3.2 rendering much tbh, but as you prefer
13:59 <reduz> this is more of a workaround for a limitation in 3.2 design which was fixed in 4.0
14:01 <Akien> Are there drawbacks to the workaround, or is it ok as band-aid until 4.0? Since people will still make and release games with 3.2 (likely many will stay on 3.2 for current games in production, since porting to 4.0 might appear as work they don't want to do)
14:03 <Akien> On the PR you mentioned that it goes against implementing batching, but that's unlikely to happen for 3.2 anyway.
14:03 <reduz> Akien: I dont even know how useful it is
14:03 <Akien> Well fracteed and a few other users seem to really want it
14:03 <Akien> #19800
14:04 <IssueBot> #19800: World_vertex_coords flag required in canvas_item shaders | https://git.io/fjxIm
14:04 <reduz> the problem is that you cant really have real world coordinates in canvas item shaders in 3.x
14:04 <reduz> I am not sure if this fixes that
14:04 <reduz> because canvas transform is preapplied to the transform
14:05 <Akien> I guess we should ask fracteed and the other interested user to test the PR and check if that does what they need?
14:05 <reduz> this solution is hacky, while in 4.0 it is like this by default
14:06 <reduz> i generally prefer to do things properly in general even if it takes longer
14:06 <Akien> Yeah but that's the engine dev perspective, not the engine user one :P
14:07 <reduz> if its up to me I would not merge it, but feel free to do as you wish
14:07 <Akien> I'll just ask the users to test it and confirm that it solves something they really need in 3.x -- otherwise we can skip it indeed.
14:08 <Akien> I think fracteed is already waiting for Vulkan for his game anyway, so he may not need this in 3.2 so badly.
14:08 <reduz> ask them if they will really use it, knowing its going to be different in 4.0 given it has world coords by default

@fracteed @bfishman Do you still need this feature, and if so, do you need it in 3.2 or can your projects wait for 4.0? And if you need it in 3.2, could you test this PR and check that it actually solves your use case?

@clayjohn I guess the bugfix you mentioned would be worth having in another PR.

@clayjohn
Copy link
Member Author

@akien-mga I made a PR for the bugfix. Let me know if we are going to merge this PR so I can rebase it with the bugfix as I fixed it in a better way than I did in this PR.

@fracteed
Copy link

@akien-mga sure I am happy to wait for 4.0 for this feature.

@bfishman
Copy link

@akien-mga personally I can wait. I had to put all non essential projects on hold because of life and don't expect to revisit until 2020. And honestly, once we have easy access to compute shaders I will end up taking a different approach for this particular use case anyhow.

Thank you all

@akien-mga
Copy link
Member

Thanks for the update. Given @reduz's reluctance with merging this for 3.2, I guess we'll skip it then.

I set #19800 to the 4.0 milestone, we'll have to confirm that it's actually fixed as expected in the vulkan branch, when it's ready for broader testing.

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.

World_vertex_coords flag required in canvas_item shaders
8 participants