Skip to content

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Feb 26, 2022

Objective

Solution

  • Add a function to just get NDC

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 26, 2022
@mockersf mockersf added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Feb 26, 2022
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.

Code-wise fine. Just @alice-i-cecile 's documentation comment to address.

@IceSentry
Copy link
Contributor

IceSentry commented Mar 2, 2022

I wouldn't mind having a description of the NDC acronym somewhere in a doc comment

nevermind, it's already there

@aevyrie
Copy link
Member

aevyrie commented Mar 6, 2022

@mockersf here are some proposed changes to this PR: mockersf#78

I was experimenting with using a trait to remove the need for passing both Image and Window resources into the world_to_screen() function.

If you have a Window or Image:

camera.world_to_screen(window, global_transform, world_pos)

If you have a RenderTarget or Camera:

let sized_target = camera.target.as_sized_target(windows, images);
camera.world_to_screen(sized_target, global_transform, world_pos)

@alice-i-cecile
Copy link
Member

I like aevyrie's changes quite a bit here. Do you agree @mockersf?

@mockersf
Copy link
Member Author

mockersf commented May 2, 2022

after playing with the changes from @aevyrie, I disagree with them as they make it possible to use invalid parameters by using a different camera for the target as the one used for the projection matrix

@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 May 2, 2022
@cart
Copy link
Member

cart commented May 3, 2022

Relevant to some of the general sentiment expressed here: in my "high level render targets" changes i'm experimenting with syncing render target logical and physical width/height to Camera fields, which makes methods like "world_to_screen" much nicer to use (and cleans up internals in a number of places). That wouldn't block anything here, just something to start thinking about / developing opinions on.

@alice-i-cecile
Copy link
Member

Relevant to some of the general sentiment expressed here: in my "high level render targets" changes i'm experimenting with syncing render target logical and physical width/height to Camera fields, which makes methods like "world_to_screen" much nicer to use (and cleans up internals in a number of places).

The main complaint I have here is the need for Assets<Images> in these APIs. I understand why it exists (because we may be rendering to something other than a window), but it's a confusing and boilerplate-heavy API. It sounds like we'll be able to clean that up with your changes, which I would really like.

@mockersf
Copy link
Member Author

mockersf commented May 3, 2022

For my use case, having a world_to_ndc works better than having to go through the screen dimensions with world_to_screen. And as a bonus you don't need the render target size 🙂

@cart
Copy link
Member

cart commented May 3, 2022

bors r+

bors bot pushed a commit that referenced this pull request May 3, 2022
# Objective

- After #3412, `Camera::world_to_screen` got a little bit uglier to use by needing to provide both `Windows` and `Assets<Image>`, even though only one would be needed https://github.com/bevyengine/bevy/blob/b697e73c3d861c209152ccfb140ae00fbc6e9925/crates/bevy_render/src/camera/camera.rs#L117-L123
- Some time, exact coordinates are not needed but normalized device coordinates is enough

## Solution

- Add a function to just get NDC
@bors bors bot changed the title simplified API to get NDC from camera and world position [Merged by Bors] - simplified API to get NDC from camera and world position May 3, 2022
@bors bors bot closed this May 3, 2022
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
…4041)

# Objective

- After bevyengine#3412, `Camera::world_to_screen` got a little bit uglier to use by needing to provide both `Windows` and `Assets<Image>`, even though only one would be needed https://github.com/bevyengine/bevy/blob/b697e73c3d861c209152ccfb140ae00fbc6e9925/crates/bevy_render/src/camera/camera.rs#L117-L123
- Some time, exact coordinates are not needed but normalized device coordinates is enough

## Solution

- Add a function to just get NDC
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…4041)

# Objective

- After bevyengine#3412, `Camera::world_to_screen` got a little bit uglier to use by needing to provide both `Windows` and `Assets<Image>`, even though only one would be needed https://github.com/bevyengine/bevy/blob/b697e73c3d861c209152ccfb140ae00fbc6e9925/crates/bevy_render/src/camera/camera.rs#L117-L123
- Some time, exact coordinates are not needed but normalized device coordinates is enough

## Solution

- Add a function to just get NDC
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-Usability A targeted quality-of-life change that makes Bevy easier to use 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