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

Polyline Fixes #711

Merged
merged 4 commits into from
Apr 30, 2013
Merged

Polyline Fixes #711

merged 4 commits into from
Apr 30, 2013

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Apr 29, 2013

This has two fixes for #695.

  • Culls polylines with width less than 1.0.
  • Set polyline bounding sphere center x coordinate to 0.0 to prevent culling by the near plane.

@@ -58,6 +58,13 @@ void clipLineSegmentToNearPlane(

void main()
{
// cull if width is less than 1.0
if (abs(texCoordExpandWidthAndShow.z) < 1.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

While I'm fine with this. From a pure technical standpoint, should this be <= 0.0 instead of < 1.0? Is a width between 0 and 1 never valid use case? @pjcozzi ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest not doing this check here at all. A more efficient and convenient solution is to write the show component in the vertex data as (show && width > 0.0). For example, see writeOriginAndShow in BillboardCollection.js.

As for widths less than 1.0, I would have to try it.

@mramato
Copy link
Contributor

mramato commented Apr 29, 2013

Other than that one question, this all looks good to me. Original problem is gone, tests pass.

@bagnell
Copy link
Contributor Author

bagnell commented Apr 30, 2013

@pjcozzi @mramato This is ready. I made it so that the width must be greater than zero instead of at least one. Let me know if that's OK.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 30, 2013

Code is OK with me. @mramato merge when you're ready. Also, we might want a test for width === 0 just like we have a test (I assume) for show === false.

@mramato
Copy link
Contributor

mramato commented Apr 30, 2013

Since Cozzi mentioned it, a test would be good. Assuming we have one for show, it should just be a copy/paste job.

@bagnell
Copy link
Contributor Author

bagnell commented Apr 30, 2013

@mramato I added a test. This is ready.

mramato added a commit that referenced this pull request Apr 30, 2013
@mramato mramato merged commit 4bcf1b8 into master Apr 30, 2013
@mramato mramato deleted the polylines branch April 30, 2013 19:20
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.

3 participants