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

Oriented bounding box & frustum culling optimizations #2782

Merged
merged 31 commits into from
Jun 25, 2015

Conversation

kainino0x
Copy link
Contributor

(Do not merge. Will be tested with 3D buildings first.)

In review & editing.

  • Further testing of rectangles which span the equator
  • Overall (rendering and network) performance benchmarking. Update the TODO in CHANGES.md.

Future:

  • Hook up to heightmapped terrain (and flat terrain, if that's separate); not just quantized meshes
  • Hook up to 2D and Columbus view rendering modes

* @type {Matrix3}
* @constant
*/
Matrix3.ZERO = freezeObject(new Matrix3(0.0, 0.0, 0.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, add this to Matrix2 and Matrix4. Mention these in CHANGES.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where in CHANGES.md should I put it? Since I'm assuming the 1.10 section is final.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make a new section for 1.11.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 3, 2015

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 3, 2015

Folks - this is the start of a pull request for more exact view frustum culling so we do not plan to merge this until we have it hooked up to terrain.

@kainino0x kainino0x force-pushed the oriented-bounding-box branch 2 times, most recently from c173ebb to de9094e Compare June 3, 2015 19:16
};

/**
* XXX Temporary compatibility function - remove after rebasing ono the rename from intersect to intersectPlane.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO for self: remove when rebasing onto global rename from intersect to intersectPlane

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 3, 2015

Should I do the intersect -> intersectPlane rename in a separate PR and rebase onto that?

It's fine to include it in this PR.

@kainino0x kainino0x force-pushed the oriented-bounding-box branch 2 times, most recently from 09e1602 to 28c8c9d Compare June 3, 2015 20:19
@kainino0x
Copy link
Contributor Author

TODO: Since we're changing intersect to intersectPlane anyway, consider replacing the Cartesian4 plane argument with an actual Plane plane.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 5, 2015

+1 on changing to Plane. This code was been written before Plane.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 5, 2015

Just double check how wide of a breaking change it becomes.

@kainino0x
Copy link
Contributor Author

@pjcozzi CullingVolume exposes a {Cartesian4[]} field that it uses internally with .intersectPlane. Nothing public needs to change except for .intersectPlane now having a different signature than .intersect. It seems that it would behoove us to add Plane.fromCartesian4 and probably Plane.UNIT_X/Y/Z or similar.

@kainino0x kainino0x changed the title Oriented bounding box Oriented bounding box & frustum culling optimizations Jun 9, 2015
@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 9, 2015

All sounds good.

@kainino0x kainino0x force-pushed the oriented-bounding-box branch 2 times, most recently from 7b56349 to 0ed700e Compare June 9, 2015 19:37
This affects bounding volumes AxisAlignedBoundingBox and BoundingSphere.
@kainino0x
Copy link
Contributor Author

There have been a few changes that should be reviewed but other than these I don't know of anything I might change.

I added an adjustment to expand the bounding box to cover the tile skirt. This seems like something we should have, although it seems to sometimes pretty significantly increase the number of tiles rendered (279 to 298 compared to 330 for the sphere in one case). Any comments?

@kainino0x
Copy link
Contributor Author

I'm working on a blog post draft here: https://github.com/kainino0x/draft-obb-blog-post

@kring
Copy link
Member

kring commented Jun 18, 2015

I added an adjustment to expand the bounding box to cover the tile skirt.

It shouldn't be necessary to account for the skirts, since they're only intended to fill the vertical space between tiles. It's hard to imagine a situation where only the skirt is visible but the rest of the tile is not. You haven't seen such a case have you?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 19, 2015

@kainino0x and I talked offline, we are not going to include the skirts, but we'll comment it in the code incase we ever see cracks.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 19, 2015

I'm working on a blog post draft here: https://github.com/kainino0x/draft-obb-blog-post

Looks great. I will do some minor edits and publish next week. Is there anything else you wanted to add/change?

@kainino0x
Copy link
Contributor Author

I'll change the numbers/screenshots a bit to reflect the removal of the skirt change. I'll push that to the repo, so you'll have it when you go to do the editing.

@kainino0x
Copy link
Contributor Author

279 to 298 compared to 330 for the sphere in one case

For the record, the case in which the skirts hurt the performance was this one: "Toward horizon, low altitude".

@kainino0x
Copy link
Contributor Author

In the blog post, should I mention why the bounding box appears not to contain the tile, because of the skirt sticking out?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 19, 2015

Yes, I think that is an interesting point to make: the box doesn't full bound the geometry, but, in practice, we don't expect it to generate artifacts since the view position/direction would never be such that it would matter.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 21, 2015

@kainino0x let me know when the blog post is ready and I will copyedit and publish this week.

Also, let me know this is ready to merge.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 22, 2015

@kainino0x also, remind me why we decided not to use the OBB for binning into multiple frustums? I want to keep some notes in case we decide to do it later.

@kainino0x
Copy link
Contributor Author

@pjcozzi I just pushed a commit to the blog post which I had forgotten to push on Friday. Blog and PR should be ready.

I don't know that we made a specific decision not to use the OBB for binning into multiple frustums. It's possible that we just decided when it came up that I should keep working on other parts first.

@kainino0x
Copy link
Contributor Author

Oh, I also forgot to push the heightmapped terrain commit to this repository on Friday. Please review that short commit. @pjcozzi

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 23, 2015

@kainino0x can you merge in master?

This looks good. @kainino0x are you OK with us merging this so it can sit in master for a few days before the 1.11 release on July 1? Any additional OBB optimizations can come in a separate PR.

@kainino0x
Copy link
Contributor Author

Sure. I'll sync with master when I get home today.

@kainino0x
Copy link
Contributor Author

Done. I'm seeing a bunch of errors but they are also there on master. Plus the timeout ones that I see regularly.

@mramato
Copy link
Contributor

mramato commented Jun 24, 2015

The failures in master are a website issue. I'm putting in a fix now.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 25, 2015

Thanks @kainino0x. If we want to tweak this before the 1.11 release, we have until early next week.

pjcozzi added a commit that referenced this pull request Jun 25, 2015
Oriented bounding box & frustum culling optimizations
@pjcozzi pjcozzi merged commit 22f7dfc into CesiumGS:master Jun 25, 2015
@pjcozzi pjcozzi deleted the oriented-bounding-box branch June 25, 2015 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants