-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GPU Picking v2 #8784
GPU Picking v2 #8784
Conversation
Example |
You added a new example but didn't update the readme. Please run |
1 similar comment
You added a new example but didn't update the readme. Please run |
Example |
1 similar comment
Example |
You added a new example but didn't update the readme. Please run |
Example |
1 similar comment
Example |
You added a new feature but didn't add a description for it. Please update the root Cargo.toml file. |
db97ffc
to
f4dc8e1
Compare
You added a new feature but didn't add a description for it. Please update the root Cargo.toml file. |
1 similar comment
You added a new feature but didn't add a description for it. Please update the root Cargo.toml file. |
You added a new feature but didn't add a description for it. Please update the root Cargo.toml file. |
Any chance that this PR will be able to get into 0.11? Looks like you have done a lot of good work on it @IceSentry and would be great to be able to use it with an actual Bevy release soon! |
It's pretty much ready, but it needs reviews and since 0.11 is so close people are focusing on older PR and getting the release ready. It will most likely get merged pretty fast after 0.11 is released though. |
Oh, just saw the conflicts, I'll need to fix that first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good! Just a few questions and remarks concerning perf/memory usage.
#ifdef GPU_PICKING | ||
out.mesh_id = mesh.id; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe update the custom material examples in addition to this example. Since it will inform anyone looking to make any kind of material. Otherwise users might end up surprised that their material doesn't work with picking.
With a separate example, someone needs to specifically be looking to make their material compatible with GPU picking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like keeping examples as single-focus as possible but I guess you're right that people making third party shader should know about this even if they don't use picking.
crates/bevy_core_pipeline/src/core_3d/main_opaque_pass_3d_node.rs
Outdated
Show resolved
Hide resolved
@@ -192,8 +194,10 @@ pub fn extract_meshes( | |||
let mut not_caster_commands = Vec::with_capacity(*prev_not_caster_commands_len); | |||
let visible_meshes = meshes_query.iter().filter(|(_, vis, ..)| vis.is_visible()); | |||
|
|||
for (entity, _, transform, previous_transform, handle, not_receiver, not_caster) in | |||
visible_meshes | |||
let mut visible_mesh_entities = vec![Entity::PLACEHOLDER]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having VisibleMeshEntities
in the render world, and pass it to the main world for each picking camera, what about just updating a VisibleMeshEntities
in the Main world?
It's a big deal, given this might be a pretty large array and it's allocated & cloned every frame (currently).
Also keeping it in the main world will not prevent you from updating it from this system (so that you still have correct ordering to match the mesh_id
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I don't remember why I did it this way 😅. I'll look into doin that instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you look into it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. I started looking into it a bit, but hit a blocker and stopped. I'm planning on trying again next week.
if entity != Entity::PLACEHOLDER { | ||
Some(entity) | ||
} else { | ||
None | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, instead of storing an Entity::PLACEHOLDER
at index zero, I would do index.checked_sub(1).map(|i| self.data.visible_mesh_entities[i])
Co-authored-by: Kaylee Simmons <[email protected]> Co-authored-by: Nicola Papale <[email protected]>
39cf863
to
7d1a5cc
Compare
The mouse position should use the physical coordinate because the readback buffer is using the physical size. // ***** now the result is correct ****//
let mouse_position = (moved_event.position.as_dvec2() * moved_event.scale_factor).as_uvec2();
for gpu_picking_camera in &gpu_picking_cameras {
// This will read the entity texture and get the entity that is at the given position
if let Some(entity) = gpu_picking_camera.get_entity(mouse_position) {
if let Some(hovered) = *hovered {
if entity != hovered {
set_color(hovered, Color::BLUE);
}
}
set_color(entity, Color::RED);
*hovered = Some(entity);
} else {
if let Some(hovered) = *hovered {
set_color(hovered, Color::BLUE);
}
*hovered = None;
}
} |
@happydpc can you make a PR to my branch with the web support |
Sure, I am still trying modify the copied buffer size, if we only want to pick the point nearby, we don't need to copy the entire texture then. |
I intentionally left that out as a feature for 2 reasons:
|
|
What is the current status of this PR? Hoping to see this merged at some point, lots of good work has been done already |
Hello! I want to ask what the current status of this PR is and how it will behave with custom shaders. Will it also be possible to render something in the picking that is not visible in the normal render mesh? For example a bigger interaction circle for mobile/touch input but without rendering that in the real scene. And it also seems like it just directly results in the entity id, I wonder if it would be easy to add support for multiple picking textures which then not directly hold the entity id but an id created with the picking texture in a follow-up PR. |
The branch is currently very outdated so I'll need to update it. It's not high on my priority list right now but I want to get back to this at some point in the next few months |
Bevy's internals have change too much since this PR and this would most likely need a rewrite. The general principle would still be the same though. This is currently low priority for me, so I'll close the PR but if anyone wants this feel free to adopt it. |
Objective
Solution
This happens over multiple frames
Frame N:
Frame N + 1:
Frame N + 2:
This works at the
Camera
level, so it will work with multiple windows or split-screen.The texture format is Rg16Uint, this means it can only render up to 65536 visible entities
Currently only implemented for 3d to keep it simple
Works with transparency, opaque, alpha mask and custom materials:
gpu_picking.mp4
Here's the mesh id texture generated for the above video
Multiple window support:
Code_LMlDGjcjWh.mp4
Open Questions
Changelog
GpuPickingPlugin
,GpuPickingCamera
,GpuPickingMesh
Future Work
Co-Authored By: @torsteingrindvik