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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raycasting #615

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Raycasting #615

wants to merge 13 commits into from

Conversation

MarekLg
Copy link
Contributor

@MarekLg MarekLg commented Oct 2, 2020

Adding this as a draft, as I made some assumptions I am unsure of:

  • adding this as a separate crate: bevy_physics
  • Ray::from_mouse_position(..) should be a method of Ray
  • the example raycast.rs is as simple as possible (e.g. the move mouse.system() could certainly be removed)

Looking forward to your guys feedback 馃槃

@memoryruins memoryruins added the C-Enhancement A new feature label Oct 4, 2020
@MarekLg MarekLg mentioned this pull request Oct 6, 2020
@bonsairobo
Copy link
Contributor

I think we probably don't need a separate crate for this. I'd just add a method to the existing Camera type. It could be really simple if the Camera already knew how big the window was, since it's a little bit inconvenient to have to look up the window as well.

Wherever Camera is assigned a window ID, I'd also copy the window size into new fields on the Camera.

As far as where to put the Ray type, probably just in bevy_math would work?

@smokku
Copy link
Member

smokku commented Dec 4, 2020

I think we probably don't need a separate crate for this. I'd just add a method to the existing Camera type. It could be really simple if the Camera already knew how big the window was, since it's a little bit inconvenient to have to look up the window as well.

Having to access the window is a bit inconvenient, but gets the current information.
Copying this info violates the DRY principle. Windows can be resized.
Encapsulating the window access inside the raycasting method removes the inconvenience.

@bonsairobo
Copy link
Contributor

@smokku

Encapsulating the window access inside the raycasting method removes the inconvenience.

How would you? I think right now it's necessary to first query for a Camera and then get the Window via Res<Windows>.

@bonsairobo
Copy link
Contributor

Copying this info violates the DRY principle. Windows can be resized.

I was trying to suggest that the size is kept up to date between the Window and Camera. I agree that ideally you wouldn't have to do this. It's just the only thing that came to mind. I'm open to other ideas. Maybe somehow SystemParam could be used to access the window in a convenient way?

@smokku
Copy link
Member

smokku commented Dec 4, 2020

Copying this info violates the DRY principle. Windows can be resized.

I was trying to suggest that the size is kept up to date between the Window and Camera. I agree that ideally you wouldn't have to do this. It's just the only thing that came to mind. I'm open to other ideas. Maybe somehow SystemParam could be used to access the window in a convenient way?

I mean, right, you have to get the Camera and Windows resources to your system, but then just pass them to the casting method and let the raycaster pull whatever information it wants.

}

pub fn from_mouse_position(
mouse_position: &Vec2,
Copy link
Member

@smokku smokku Dec 4, 2020

Choose a reason for hiding this comment

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

Window now has .cursor_position() method. We could add:

pub fn from_window(
    window: &Window,
    camera: &Camera,
    camera_transform: &GlobalTransform,
) -> Self {
    Self::from_mouse_position(window.cursor_position(), camera, camera_transform)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. I'll add it.


let hit = if let Some(plane_hit) = plane_hit {
if let Some(sphere_hit) = sphere_hit {
if plane_hit.t() < sphere_hit.t() {

Choose a reason for hiding this comment

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

We have 6 nested (for and if) statements here, that is very complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I disagree with 6 nested blocks being inherently complex, I agree that this particular instance is unnecessarily verbose and doesn't get the point across. I'm looking for something more elegant but feel free to propose changes where you see fit.

use glam::Vec3;

pub struct RayHit {
t: f32,

Choose a reason for hiding this comment

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

Please consider renaming it to something more meaningful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea!


impl RayIntersector for Plane {
fn intersect_ray(&self, ray: &Ray) -> Option<RayHit> {
let d = self.normal().dot(*ray.direction());

Choose a reason for hiding this comment

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

Please describe what is this line doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It assigns the dot product between the plane normal and the ray direction to the variable d (now changed to denominator). I don't think that needs a comment.

let d = self.normal().dot(*ray.direction());
if d.abs() > f32::EPSILON {
let t = (*self.center() - *ray.origin()).dot(*self.normal()) / d;
if t > 0.0 {

Choose a reason for hiding this comment

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

I am guessing the t and d is taken from some formula. We can give it a more verbose name in our code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree and have changed the names.

camera_transform: &GlobalTransform,
) -> Self {
if window.id() != camera.window {
panic!("Generating Ray from Camera with wrong Window");

Choose a reason for hiding this comment

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

Correct me if i'm wrong, this can terminate the program?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can, you're right. This is by design. Passing a non-matching window and camera to from_mouse_position(..) would result in undefined behavior and should be checked before calling the method. Do you think a Log Error would be more sensible? I didn't familiarize myself very much with the Diagnostics Plugin, so maybe that could help.

Choose a reason for hiding this comment

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

TBH I don't know the right answer, I am learning here 馃憤

}
}
}

impl RayIntersector for Triangle {
// using the Moeller-Trumbore intersection algorithm
// Can anyone think of sensible names for theese?
#[allow(clippy::many_single_char_names)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this allow acceptable?

@@ -54,6 +54,8 @@ struct MouseState {
cursor_position: Vec2,
}

// Is there a way to reduce the arguments?
#[allow(clippy::too_many_arguments)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this allow acceptable?

@alice-i-cecile
Copy link
Member

We have a community crate for this now as well: bevy_mod_raycast. It has significantly more functionality, and I've recently done a full code review on it for quality.

I definitely think this functionality is useful though, and I'm of the opinion that raycasting probably deserves a home in bevy itself once it matures a bit more and we want to build functionality (like the editor, PBR or convenience functions) on top of raycasting.

@smokku
Copy link
Member

smokku commented Jan 24, 2021

I'm not sure merging everything under Bevy umbrella is a sustainable approach. This creates a contract for supporting these things.

Maybe it would be better to provide an easy way of adding&integrating 3rd party plugins - like a "plugin store" etc.

@cart
Copy link
Member

cart commented Jan 25, 2021

Yeah I've been holding off on this because first party raycasting isn't a priority and its a relatively opinionated feature. In the short term, do I think it makes sense to let the community handle raycasting in 3rd party plugins.

Eventually I think we'll need to adopt something officially, at least for the editor. But I think that raycasting is a common enough operation that we will want an official general-purpose solution (in the long term).

I also agree that a "plugin store" is a good idea. Crates.io isn't great for Bevy plugin discovery and awesome-bevy isn't great at sorting / prioritizing / finding information.

I think eventually we should migrate awesome-bevy to a more interactive / informative alternative on bevyengine.org. It would basically be a database of crates.io links with additional metadata (images, category, etc) and a searchable interface. I can't say I will prioritize that in the short term though :)

Base automatically changed from master to main February 19, 2021 20:44
@bonsairobo
Copy link
Contributor

@cart Looking back on this, I still want to push for having 1st party support for at least the bare-minimum functionality that calculates a ray (position, vector) from screen coordinates and a camera (transform, projection). This math is not trivial and can vary based on how the engine implements cameras.

I agree that doing much more than that is probably not a high priority.

@alice-i-cecile
Copy link
Member

@bonsairobo I agree with the need for first-party support. I expect that @aevyrie will be working on a raycasting RFC, following up on the geometric primitives chain of work.

@aevyrie
Copy link
Member

aevyrie commented Mar 24, 2021

Thanks for the ping. 馃槃

In short, I agree with:

I'm not sure merging everything under Bevy umbrella is a sustainable approach. This creates a contract for supporting these things.

If you have suggestions for API/features in bevy_mod_raycast, I'm always happy to work with contributions, or you could fork it and start up an alternative crate to try out something different!

Until the feature becomes a priority for the engine, we can continue to experiment in external crates without burdening the engine with more code to support.

@alice-i-cecile alice-i-cecile added the S-Needs-RFC This issue or PR is particularly complex, and needs an approved RFC before it can be merged label Apr 23, 2021
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@DJMcNab DJMcNab removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Aug 7, 2021
@alice-i-cecile alice-i-cecile added S-Controversial This requires a heightened standard of review before it can be merged A-Math Fundamental domain-agnostic mathematical operations labels Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Enhancement A new feature S-Controversial This requires a heightened standard of review before it can be merged S-Needs-RFC This issue or PR is particularly complex, and needs an approved RFC before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants