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

Opt-out Z interpolation #715

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

bjornharrtell
Copy link
Contributor

ref #714

Wanted to see what effect it would have to turn interpolation off by default and it causes a number of expected test failures (OverlayNGZTest, RobustLineIntersectorZTest and GeometryFixerTest).

If we agree that #714 is indeed a regression then I would have no better current idea than doing something like this, unfortunately.

@dr-jts
Copy link
Contributor

dr-jts commented May 3, 2021

Using a global option set via System property seems like the simplest way to allow a choice of Z-interpolation behaviour (although it's the least flexible).

The unit test issues can be handled by either making Z-interpolation the default, or by changing the option setting in the unit test. The latter approach is already used in the GeometryOverlayTest.

@dr-jts dr-jts changed the title Opt in interpolation for RobustLineIntersector Opt-in Z interpolation for RobustLineIntersector May 3, 2021
@bjornharrtell
Copy link
Contributor Author

@dr-jts I've opted in the interpolation in the test cases where it is the assumption.

@bjornharrtell bjornharrtell changed the title Opt-in Z interpolation for RobustLineIntersector Opt-in Z interpolation May 4, 2021
@bjornharrtell
Copy link
Contributor Author

I've now reworked this so that the option to Z-interpolate is standalone and affects both RobustLineIntersector and ElevationModel.

* @author bjornharrtell
*
*/
public class ZInterpolating {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a lot of boilerplate to hold a boolean value decision, can this class take on more responsibility and swap between interpolation approaches as a stratagy object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it feel like boilerplate. I have difficulties properly understanding/interpreting what you mean with "take on more responsibility and swap between interpolation approaches as a stratagy object". In either case it's intentionally done this way because it is modeled after the other existing system property option jts.overlay and broken out into a self contained class because it is affecting two very separate things (RobustLineIntersector and ElevationModel), and I wanted to do it in a way that is as non intrusive as possible on the existing source.

I suppose system wide settings could be done with smarter and nicer desig but I don't think this is the place to do such general improvement and restructuring.

@bjornharrtell
Copy link
Contributor Author

Since Z interpolation has now been enabled for so long time it's no longer sensible to regard it as a regression. So I've changed it to be default on in this PR. This means this PR now do not change any behaviour and only allows the interpolation to be opt out.

@bjornharrtell bjornharrtell changed the title Opt-in Z interpolation Opt-out Z interpolation Oct 28, 2022
@bjornharrtell
Copy link
Contributor Author

@jodygarnett and @dr-jts please consider accepting this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants