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

Don't throw out short polylines #3305

Merged
merged 7 commits into from
Dec 10, 2015
Merged

Don't throw out short polylines #3305

merged 7 commits into from
Dec 10, 2015

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Dec 8, 2015

Made the epsilon values smaller for Polylines, Corridors and PolylineVolumes

This forum post reported a problem with the PolylineVolumes

@mramato
Copy link
Contributor

mramato commented Dec 8, 2015

@bagnell does the relative epsilon stuff you put in a while ago come into play here, or is this really just a case of the tolerance not being high enough?

Related question: Do we always render the polyline if there are only two points or do we end up deduping to 1 point and then throwing it out?

@hpinkos
Copy link
Contributor Author

hpinkos commented Dec 8, 2015

@mramato we'll throw it out if the polyline only has two points and the points are almost the same

@hpinkos
Copy link
Contributor Author

hpinkos commented Dec 9, 2015

Fixes #3293

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 9, 2015

Update CHANGES.md.

@bagnell can you please review?

@hpinkos
Copy link
Contributor Author

hpinkos commented Dec 9, 2015

@bagnell this is ready now.

@bagnell
Copy link
Contributor

bagnell commented Dec 10, 2015

does the relative epsilon stuff you put in a while ago come into play here

@mramato Yes, it was changed to check the absolute or relative error. Here it would check the relative error. We need to go through and check all of the epsilon values, but I think we are just fixing them as issues come up.

bagnell added a commit that referenced this pull request Dec 10, 2015
@bagnell bagnell merged commit c796c61 into master Dec 10, 2015
@bagnell bagnell deleted the polylineEpsilon branch December 10, 2015 16:55
@mramato
Copy link
Contributor

mramato commented Dec 10, 2015

Should there be a constant with a single epsilon defined somewhere that all of the geometry uses? Should that be user settable?

@mramato
Copy link
Contributor

mramato commented Dec 10, 2015

And is 10 the right answer, or just a random guess?

@hpinkos
Copy link
Contributor Author

hpinkos commented Dec 10, 2015

@mramato 10 is a random guess. It's probably small enough, but at some point we should probably figure out what's the smallest distance that's still worth rendering. 6 was definitely too big because you can still see things at that size even without being zoomed in all of the way.

Should there be a constant with a single epsilon defined somewhere that all of the geometry uses? Should that be user settable?

I don't think it would need to be user settable, but having a constant somewhere that all of the geometries use is a good idea. I added a note to #2364

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.

4 participants