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

Generic 3D bounds #187

Merged
10 commits merged into from Mar 15, 2015

Conversation

@kvark
Copy link
Collaborator

commented Mar 13, 2015

Closes #117
Fixes #125

Infrastructure to do frustum culling based on Sphere or Aabb bounds. More can be implemented for the Bound trait. Example code:

let projection = PerspectiveFov {
        fovy: rad(1f32),
        aspect: 1f32,
        near: 1f32,
        far: 10f32,
};
let frustum = projection.to_frustum();
let bound = Sphere {
    center: Point3::new(0f32, 3f32, -5f32),
    radius: 1f32,
};
match frustum.contains(&bound) {
    Relation::In => show(),
    Relation::Cross => show_anyway(),
    Relation::Out => cull(),
}
@@ -8,7 +8,7 @@
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// distributed under the License is distributed on an "AS IS" BASisize,

This comment has been minimized.

Copy link
@brendanzab

brendanzab Mar 14, 2015

Collaborator

oopsie

This comment has been minimized.

Copy link
@kvark

kvark Mar 14, 2015

Author Collaborator

nice find ;) fixed now!

@ghost ghost referenced this pull request Mar 15, 2015

Merged

Manually implment Rand. #188

assert_eq!(aabb.volume(), 30is * 40is);
assert_eq!(aabb.center(), Point2::new(-5is, 10is));
fn test_general() {
let aabb = Aabb2::new(Point2::new(-20isize, 30isize), Point2::new(10isize, -10isize));

This comment has been minimized.

Copy link
@ghost

ghost Mar 15, 2015

Thanks for cleaning this up.

// if we see Cross, then the result is Cross
// if we see In, then we keep the old result
// otherwise, take the current result
if cur == Relation::Cross || r == Relation::In {

This comment has been minimized.

Copy link
@ghost

ghost Mar 15, 2015

I think there is a bug here, if r is Relation::Cross and cur is In the state will be Relation::In instead of Relation::Cross

This might be nicer as a match too:

match (cur, r) {
    (Relation::Out, _) | (_,  Relation::Out) => Relation::Out,
    (Relation::Cross, _) | (_,  Relation::Cross) => Relation::Cross,
    _ => Relation::In
}

This comment has been minimized.

Copy link
@kvark

kvark Mar 15, 2015

Author Collaborator

The condition cur == Cross || r == In will be false, so the outcome will take the other arm, which is just r, which is exactly Cross. So I think condition is still correct?

This comment has been minimized.

Copy link
@ghost

ghost Mar 15, 2015

You are right. What about about the case where cur == Cross && r == Out?

This comment has been minimized.

Copy link
@kvark

kvark Mar 15, 2015

Author Collaborator

cur == Cross would let it go the first arm, which results in cur, which is Cross. I believe this is still correct?

This comment has been minimized.

Copy link
@ghost

ghost Mar 15, 2015

I think there are two cases missing here. If cur == Out and r == In The result should still be Out where it will be marked as In. The other case if cur == Cross && r == Out the result should be once again Out where it would be marked as Cross.

This comment has been minimized.

Copy link
@kvark

kvark Mar 15, 2015

Author Collaborator

cur == Out && r == In would result in Out, since it's the first arm of the condition. You are correct about the second case though, Cross shouldn't overwrite Out, and I need to fix the condition.

This comment has been minimized.

Copy link
@kvark

kvark Mar 15, 2015

Author Collaborator

I fixed the condition now. Thank you for spotting the logical error. Your attention to detail is highly professional!

fn relate_clip_space(&self, projection: &Matrix4<S>) -> Relation {
use std::cmp::Ordering::*;
let p = projection.mul_v(&self.to_homogeneous());
match (p.x.abs().partial_cmp(&p.w), p.y.abs().partial_cmp(&p.w), p.z.abs().partial_cmp(&p.w)) {

This comment has been minimized.

Copy link
@ghost

ghost Mar 15, 2015

Should we handle the NaN case?

This comment has been minimized.

Copy link
@kvark

kvark Mar 15, 2015

Author Collaborator

It would just return Cross for the matter of Relation. Not sure what would be a better way to handle it.

@ghost

This comment has been minimized.

Copy link

commented Mar 15, 2015

This needs to be rebased.

@kvark kvark force-pushed the kvark:bound branch from 737bd8c to 6691dce Mar 15, 2015

@kvark

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 15, 2015

Rebased now.

@kvark

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 15, 2015

We'd want to implement relate_clip_space for Aabb3 in an efficient way, described here:
https://fgiesen.wordpress.com/2010/10/17/view-frustum-culling/

But that could be done later on without breaking the interface.

@ghost

This comment has been minimized.

Copy link

commented Mar 15, 2015

I'm not sure why Travis is not building this...

@kvark

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 15, 2015

It was a bad merge on my part. The upcoming build should be fine though.

@kvark

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 15, 2015

Another thing to do would be adding a benchmark for frustum culling. This would be handy for the optimized relate_clip_space, so will probably be implemented with it.

ghost pushed a commit that referenced this pull request Mar 15, 2015

Merge pull request #187 from kvark/bound
Generic 3D bounds

@ghost ghost merged commit de2c032 into rustgd:master Mar 15, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ghost

This comment has been minimized.

Copy link

commented Mar 15, 2015

Thanks~

@kvark kvark deleted the kvark:bound branch Mar 15, 2015

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.