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

Fix GroundPrimitive clipping issue #3709

Merged
merged 4 commits into from
Mar 17, 2016
Merged

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Mar 15, 2016

Fixes #3706.

Removes the OBB optimization for better culling. The bounding volume was
pulling the far plane closer to the camera and causing the shadow volume
to be clipped.

bagnell added 2 commits March 15, 2016 13:50
pulling the far plane closer to the camera and causing the shadow volume
to be clipped.
@mramato
Copy link
Contributor

mramato commented Mar 15, 2016

I can confirm the issue is fixed. I also wrote up #3710, which is most likely unrelated.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 15, 2016

@bagnell if I'm following you correctly, the bounding volume was smaller than the shadow volume so the shadow volume was getting clipped, right?

I'm fine with this change for now since we have bigger fillrate improvements to make here, but two questions:

  • Why doesn't depth clamping clamp the clipped volume to the near/far plane?
  • Wouldn't it be more efficient to just also make the shadow volume smaller instead of making the bounding volume big? Is that just too ugly?

@bagnell
Copy link
Contributor Author

bagnell commented Mar 15, 2016

Why doesn't depth clamping clamp the clipped volume to the near/far plane?

I forgot about the near/far plane clipping. The volume doesn't need to be clipped to the near plane. It should be getting clipped to the far plane though so I'll look into it.

Wouldn't it be more efficient to just also make the shadow volume smaller instead of making the bounding volume big?

Yes, I wish we could. Most likely, we would need support from the terrain server to get the max/min height of the terrain over an area.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 15, 2016

Wouldn't it be more efficient to just also make the shadow volume smaller instead of making the bounding volume big?

Yes, I wish we could. Most likely, we would need support from the terrain server to get the max/min height of the terrain over an area.

In the general case, I agree. In this case, don't we know the max height of the smaller OBB? Couldn't we use that?

@bagnell
Copy link
Contributor Author

bagnell commented Mar 16, 2016

In this case, don't we know the max height of the smaller OBB?

Which OBB? We compute the one for the primitive based on the volume. For the terrain tile, we would have to go through the quadtree. The volume might overlap more than one tile. We can't use the parent tiles either. The min and max height of the tiles are for the quantized mesh so the min/ max heights of the parent aren't guaranteed to bound those of the children.

@kring
Copy link
Member

kring commented Mar 16, 2016

In the quantized mesh format, at least the one produced by STK Terrain Server and at least when I wrote it, the min/max heights of a tile very intentionally represent the the min/max height of that tile and all its descendants.

@bagnell
Copy link
Contributor Author

bagnell commented Mar 17, 2016

That would be great. We might be able to do something client side in the short term to shrink the volumes. I had an offline discussion with Alex where he said that was not the case. Maybe something changed? CC @abwood

@abwood
Copy link
Contributor

abwood commented Mar 17, 2016

Actually, I think Kevin is correct here. While minheight and max height is updated during refinement (which could simplify out the minheight and maxheight of the children) we do set update the parent tiles min and max before aggregating the children.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 17, 2016

@bagnell is there anything you want to do in the short-term or should we merge this?

@bagnell
Copy link
Contributor Author

bagnell commented Mar 17, 2016

@pjcozzi I think we should merge this.

pjcozzi added a commit that referenced this pull request Mar 17, 2016
…clipping

Fix GroundPrimitive clipping issue
@pjcozzi pjcozzi merged commit 00ba3b6 into master Mar 17, 2016
@pjcozzi pjcozzi deleted the ground-primitive-clipping branch March 17, 2016 18:21
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.

Ground primitive clipping issue
5 participants