Pick instance feature #479
Conversation
|
@thatcomputerguy0101 I made a separate PR for the shader ID refactor to make it easier to review and test that change separately. I'll update this PR after that is merged. |
thatcomputerguy0101
force-pushed
the
pick_instance_feature
branch
from
f9c557d
to
a302f37
Compare
|
This PR has now been updated to be based on the current master branch. |
There was a problem hiding this comment.
Super nice with an example, but maybe we can just extend the current picking example to also change the color of the selected mesh/instance.
| let mut sphere_instances = Instances { | ||
| transformations: [ | ||
| Mat4::from_translation(vec3(2.0, 0.0, 0.0)), | ||
| Mat4::from_translation(vec3(4.0, 0.0, 0.0)), |
There was a problem hiding this comment.
Would be nice with a lot of instances.
There was a problem hiding this comment.
I am confused if this is a suggestion for a change or just a comment.
There was a problem hiding this comment.
It is a suggestion, currently, there's only two instances, it's better to test with 100 instances for example.
| /// | ||
| /// Representation of a specific geometry instance | ||
| /// | ||
| pub struct GeometryInstance { |
There was a problem hiding this comment.
I'm not sure I like the name (naming is hard). It's not a geometry instance, it's not even necessarily an picking result containing an instance. Maybe PickResult or something?
There was a problem hiding this comment.
Yeah, it used to fit the GeometryInstance name better, but has since been adjusted away from that. However, ray_intersect_instance also returns that type, so maybe IntersectionResult would be good?
| /// and (viewport.x + viewport.width, viewport.y + viewport.height) indicate the top right corner. | ||
| /// Returns ```None``` if no geometry was hit between the near (`z_near`) and far (`z_far`) plane for this camera. | ||
| /// | ||
| pub fn pick_instance( |
There was a problem hiding this comment.
These functions are almost identical to pick and ray_cast, maybe just update those return more data. It's a breaking change, but an easy one to understand and fix. Also, it shouldn't affect performance, so even if you only want the picked position, the new functionality should still be good.
There was a problem hiding this comment.
I'll probably change that with my next set of edits. (Technically it is a couple of instructions slower due to moving more data around, but that is probably a negligible difference especially once compiler optimizations are applied.)
src/renderer.rs
Outdated
| texture.as_color_target(None), | ||
| depth_texture.as_depth_target(), | ||
| ) | ||
| .clear(ClearState::color_and_depth(-1.0, -1.0, -1.0, -1.0, 1.0)) |
There was a problem hiding this comment.
The clear values are clamped between 0 and 1, so this doesn't make sense.
There was a problem hiding this comment.
The platform that I was testing on doesn't seem to be clamping it, which was why I thought it was fine. I'll try and find an alternative solution to have the intended behavior on all platforms.
There was a problem hiding this comment.
Hmm. That's weird, but yeah, you never know how OpenGL specs are actually implemented. I guess you can use 0 for no hit and just offset the ID result with -1.
| #[derive(Default, Clone)] | ||
| pub struct GeometryInstanceMaterial { | ||
| /// Geometry ID | ||
| pub id: usize, |
There was a problem hiding this comment.
I generally try to use u32 or u16 to make it explicit. usize varies with operating system and is usually u64 which can't fit in a f32 anyway.
|
I'm looking at using ints the whole way through the GPU side, although that requires using an integer output texture which restricts support to OpenGL 3.0 or greater and WebGL 2. Would these be acceptable versions to still replace the original |
Yes, that would be ok. Would you write the depth to an integer then? You need to write depth (f32) and ID (u32) to a render target, so you have to choose either a float or integer target. I'm not sure it makes a big difference whether you choose one over the other. |
|
Part of that change would be reading the depth from the depth buffer (supported in WebGL 2 if I interpreted it correctly), which is always a float data type. |
thatcomputerguy0101
force-pushed
the
pick_instance_feature
branch
2 times, most recently
from
867cc4b
to
d5cf6c2
Compare
|
Seems like I misinterpreted being able to read from the depth texture on web. I still have some ideas to work around that though. |
|
GL's |
thatcomputerguy0101
force-pushed
the
pick_instance_feature
branch
from
d5cf6c2
to
5d4ce11
Compare
thatcomputerguy0101
force-pushed
the
pick_instance_feature
branch
from
5d4ce11
to
1d650d8
Compare
This adds
pick_instanceandray_intersect_instancefunctions to allow detecting which geometry instance a ray intersects with. There is a new examplepicking_instancesthat demonstrates this capability for basic geometry, instanced geometry, and imported models. The additional example has yet to be added to the examples README. This closes #467.Additionally, the shader IDs have been separated out to simplify keeping track of which ID's were used, and a few ID overlaps were corrected. The shader IDs internally make use of
open_enumtypes, which could be changed to also be the public type signature in a breaking release. The shader ID changes can be seen individually at https://github.com/thatcomputerguy0101/three-d/tree/shader_ID_refactor, and I can create a separate pull request for that or remove those changes if desired.