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

Building on #1799 : Add a function for computing a world point from a screen point #4177

Closed
wants to merge 7 commits into from

Conversation

omarbassam88
Copy link

Objective

Solution

  • Adding a Camera method to compute a world point from screen point.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 10, 2022
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen A-Transform Translations, rotations and scales 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 Mar 10, 2022
@omarbassam88 omarbassam88 changed the title Screen to world Building on #1799 : Add a function for computing a world point from a screen point Mar 10, 2022
@alice-i-cecile alice-i-cecile self-requested a review March 10, 2022 23:08
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 will be useful for sure. :)

I didn't manage to understand your maths in some places. It could have been correct, but I didn't understand it. Specifically world_ray.point.extend(1.0) - plane.normal_d() is very confusing to me.

I have made a couple of suggestions for changes but they are completely untested. Please check whether they work correctly. Poke me (robswain on Discord) if you need support to get them working.

crates/bevy_render/src/camera/camera.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/camera.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/camera.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/camera.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/camera.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/camera.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/camera.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/camera.rs Outdated Show resolved Hide resolved
examples/2d/mesh2d_manual.rs Outdated Show resolved Hide resolved
examples/2d/mesh2d_manual.rs Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

@inodentry @aevyrie could I get your reviews on this? I'd like to be able to finally add functionality for this into Bevy itself.

@superdump
Copy link
Contributor

I want to give this another review pass.

@inodentry
Copy link
Contributor

I think this implementation is quite inefficient for the 2D case.

The 2D functions are built on top of the 3D functions, which ... I guess ... has a kind of "elegance" to it in terms of code reuse ... but is really excessive computationally. There is no need to effectively do several matrix multiplications and a ton of linear algebra to map a screen point to a 2D world point.

See the current cheatbook example. In terms of the computations done, it is just one multiplication to go from NDC to world. The code in this PR results in 4 matrix multiplications and a bunch of vector math, just for the sake of reusing the more general screen_to_world_ray for the 2D case.

@alice-i-cecile
Copy link
Member

Should we merge in #4041 first and then use that in this PR?

@@ -192,6 +192,18 @@ impl CubemapFrusta {
}
}

#[derive(Clone, Copy, Debug, Default)]
pub struct Line {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should provide the result as a pair of Vec3's, and not a Line struct.

  1. Based on my previous contributions, cart's preference has been to not wrap in geometric structs
  2. We have a geometric primitive RFC that this may conflict with.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have very similar code to what's here, and I use a Ray<Vec3>:

pub struct Ray<V> {
    pub origin: V,
    pub direction: V,
}

A ray is useful, because the math is simpler for intersections, and the equation is self.origin + t * self.direction. I use them often. It's also useful to implement Mul<Ray<Vec3>, Output=Ray<Vec3>> for transforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Shapes RFC also has rays.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed a ray is more apt here. My opinion is we defer adding primitives to the RFC impl, especially considering we don't have cart's final blessing.

// the ray, because ortho cameras have a focal point at infinity!
let inverse_projection = projection_matrix.inverse();
let cursor_pos_view_near = inverse_projection.project_point3(cursor_pos_ndc_near);
let cursor_pos_view_far = inverse_projection.project_point3(cursor_pos_ndc_far);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this projecting to infinity, since the far plane is at -Infinity Z for reversed-Z perspective projection?

Copy link
Member

@aevyrie aevyrie Apr 25, 2022

Choose a reason for hiding this comment

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

I just noticed this is taken directly from an old version of bevy_mod_raycast, which does not work with the new projections for the reason you stated.

Here is the current version of that function that works with bevy main: https://github.com/aevyrie/bevy_mod_raycast/blob/main/src/primitives.rs#L180-L217

Copy link
Member

Choose a reason for hiding this comment

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

Also, the code "taken" is from a repo with an MIT license. I could re-license that repo as MIT/Apache if I get permission from contributors, but I'm not very familiar with licensing laws. The math here is well understood and fairly trivial, the only issue I can see is there was an obvious copy-paste.

Copy link
Contributor

Choose a reason for hiding this comment

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

A simpler solution is to use z = 1.0 for the near plane and z = 0.5 for half-way to the far plane (just kidding, it works though.)

@aevyrie
Copy link
Member

aevyrie commented Apr 25, 2022

I spoke with @alice-i-cecile about this, I'm closing for a few reasons:

  1. The screenspace conversion method used here doesn't work since the change to an inf-reversed projection
  2. Some code in this PR is copied directly from an MIT-licensed repo. I own the repo, but the file it is taken from has multiple contributors, meaning we would probably need to go through re-licensing steps
  3. This is a small enough PR it will be much less messy if it is re-implemented from scratch.

@aevyrie aevyrie closed this Apr 25, 2022
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 A-Transform Translations, rotations and scales C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants