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

View Transformations #9726

Merged
merged 16 commits into from
Oct 24, 2023
Merged

Conversation

DGriffin91
Copy link
Contributor

Objective

  • Add functions for common view transformations.

@DGriffin91 DGriffin91 mentioned this pull request Sep 8, 2023
@alice-i-cecile
Copy link
Member

Same comment as on #9718 (which should be coordinate with this): is there a reason why these need to be tied to PBR?

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Sep 8, 2023
@IceSentry
Copy link
Contributor

I'll answer here because I think this PR supersedes the other PR.

why these need to be tied to PBR?

Right now, bevy_pbr == bevy_3d. There's a future where we split them and pbr is strictly limited to pbr stuff but for now you can't do 3d without bevy_pbr.

@DGriffin91
Copy link
Contributor Author

@alice-i-cecile I would love for this to live outside bevy_pbr but unfortunately with the limitations of our binding system and view_transformations's dependence on bevy_pbr::mesh_view_bindings that isn't currently possible without significantly hurting the ergonomics.

@IceSentry
Copy link
Contributor

Yeah, a part of me would like to have those functions that don't assume the view bindings are present but it would indeed make using them way more annoying

@JMS55
Copy link
Contributor

JMS55 commented Sep 8, 2023

Please make View an argument you can pass (view: ptr<function, View>). A lot of the time you want to use these functions, you won't be using the pbr mesh view bindings.

Similarly, I don't see why we couldn't move this to bevy_render.

Please also update gtao.wgsl to use these functions and remove the old code there.

@IceSentry
Copy link
Contributor

IceSentry commented Sep 8, 2023

I guess one potential solution would be to have the bindings agnostic functions defined in bevy_render and the more ergonomic version defined in bevy_pbr

@DGriffin91
Copy link
Contributor Author

DGriffin91 commented Sep 8, 2023

@JMS55 Unfortunately you can't pass uniforms as ptr. Here are various possible solutions:

// 1. Current PR
let ws_pos = position_ndc_to_world(vec3(uv_to_ndc(uv), depth));

// 2. Pass in view ptr
var view_temp = view_bindings::view;
let view = &view_temp;
let ws_pos = position_ndc_to_world(view, vec3(uv_to_ndc(uv), depth));
    
// 3. Pass in mat
// Nothing here will keep users from entering the incorrect view field.
let ws_pos = position_ndc_to_world(view.inverse_view_proj, vec3(uv_to_ndc(uv), depth));

// 4. Compute directly but with updated view mat names
let temp = view_bindings::view.ndc_to_world * vec4(uv_to_ndc(uv), depth, 1.0);
let ws_pos = temp.xyz / temp.w;

Personally I think 1 and 4 are ok.

@JMS55
Copy link
Contributor

JMS55 commented Sep 8, 2023

I personally like 1. or 3. The problem is 1. doesn't work for things like SSAO. Imo, with doc comments, 3. is fine.

@DGriffin91
Copy link
Contributor Author

3 to me is somewhat nonsensical. The contents of every position_to_* function would be exactly the same except for ones involving clip. Same for all the direction_to_* functions.

If users are going to have to read docs to use these then they can use 4.

@JMS55
Copy link
Contributor

JMS55 commented Sep 8, 2023

In that case I'm in favor of just 1., and at least we have this documented so I can just copy paste the code when I need it elsewhere :)

@superdump
Copy link
Contributor

Please do not merge this until I have reviewed it.

@superdump superdump self-requested a review September 9, 2023 05:21
@JMS55 JMS55 added this to the 0.12 milestone Sep 28, 2023
@alice-i-cecile alice-i-cecile removed this from the 0.12 milestone Sep 30, 2023
@superdump superdump added this to the 0.12 milestone Oct 8, 2023
@superdump
Copy link
Contributor

Adding back to 0.12 milestone as a dependency of #9258 .

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is one perspective divide that is out of place.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a complete review pass. I'm highlighting a sticking point that we need to straighten out and align on.

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I've been using the code from this PR in a couple of side projects and it worked really well.

LGTM assuming conflicts are fixed of course.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 24, 2023
@superdump superdump added this pull request to the merge queue Oct 24, 2023
Merged via the queue into bevyengine:main with commit 1bd7e5a Oct 24, 2023
22 checks passed
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

- Add functions for common view transformations.

---------

Co-authored-by: Robert Swain <[email protected]>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Add functions for common view transformations.

---------

Co-authored-by: Robert Swain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants