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

Add Z handling to LineSegment and Densifier #658

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

Conversation

mukoki
Copy link
Contributor

@mukoki mukoki commented Dec 31, 2020

Take care of z ordinate in LineSegment. Also benefits Densifier which can now interpolate z values. A hasZ() method has been added in different coordinates implementation : it is not strictly needed but is more readable than cryptic Double.isNaN(d)

@dr-jts
Copy link
Contributor

dr-jts commented Jan 15, 2021

This is certainly a very thorough PR. However, I am not sure that LineSegment should automatically assume that calculations should be carried out in 3D. Often geometry is modelling "2.5D", where the Z value represents a value which is separate to the base 2D plane on which the geometry is embedded.

An alternative is to implement new 3D methods on LineSegment which have the behaviour supplied in this PR (e.g. pointAlongOffset3D, etc).

Also, Densifier could provide an option (say use3D) to explicitly specify if the calculations should take 3D into account.

@dr-jts dr-jts changed the title Coordinate dimension Add Z handling to LineSegment and Densifier Jan 15, 2021
@mukoki
Copy link
Contributor Author

mukoki commented Jan 15, 2021

I know JTS is a 2D library by design and z (or m) values will always have a place appart,
but Coordinate has a z attribute and it is the one used to modelize a CoordinateXYZ as there
is no CoordinateXYZ subclass of Coordinate. So, throwing the z attribute away during calculation
often lead to bad surprises.
Note that in my proposition, I did not go all the way long to process the whole hierarchy of
Coordinates as I did not care of the cases where Coordinate is, indeed, a XYM or a XYZM Coordinate.
I choose to align the change to the one you did recently to interpolate z values during overlay
computation (which, for me, is a very welcome change as I sometime had to use postgis/geos instead
of JTS just for that feature).
If the model had a 2D Coordinate base class and XYZ, XYM, XYZM subclasses, maybe there would be less
ambiguities, but with a base class having x,y,z attributes actually used to model CoordinateXYZ,
how to ignore the z value?
Also : interpolating the z value during LineSegment computation does not make many hypothesis about
what the z attribute contains. It is a bit of computation, but it should work whatever the z value contains
and even if it does not contain z (NaN) as in the case of z interpolation in the new overlay.
Do you have in mind some cases where z value is used to modelize something else that a value to be
interpolated and where it is better to not keep it in the returned values ?
Implementing z interpolation in an alternate pointAlongOffset3D is probably possible, but now that we have a hierarchy of coordinates, making z interpolation an option even for input coordinates embeding a z value is a bit of overhead..

@dr-jts
Copy link
Contributor

dr-jts commented Jan 15, 2021

My main concern is changing behaviour of LineSegment calculations in unanticipated ways. Putting the computations involving Z into new methods would avoid this risk.

I'm not so concerned about Densifier. Although I suspect that Densifier is mostly used to alleviate problems with algorithms which are themselves 2D only, and hence using the Z value might also produce unanticipated results. E.g. one use for Densifier is to allow long line segments to "bend" during reprojection - and reprojection is 2D only, typically.

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.

2 participants