jts icon indicating copy to clipboard operation
jts copied to clipboard

Shouldn't 4d coordinates always be XYZM ?

Open mukoki opened this issue 5 years ago • 12 comments

Nothing states that fourth dimension of a Coordinate and followings must be measures.

So, at first glance, it seems consistent to not set measures if not specified in the two arguments constructors as in :

public create(Coordinate[] coordinates, int dimension) {
  this( coordinates, dimension, 0);
}

Problem is that if the user build a 4d CoordinateSequence using a constructor with only the dimension parameter, he isn't warned that he will soon loose the fourth dim of his Coordinates as in CoordinateSequence.toCoordinates() (because of PackedCoordinateSequence.getCoordinateInternal() implementation)

=> To make things simpler, I would be in favour of always interpreting dimension = 4 as XYZM and, by generalization to consider that all ordinates following z are measures.

public create(Coordinate[] coordinates, int dimension) {
  this( coordinates, dimension, Math.max(0, dimension-3));
}

I can provide a patch and a test for that if needed.

mukoki avatar Aug 15 '20 13:08 mukoki

Seems like a valid observation. A test would help to clarify.

dr-jts avatar Aug 27 '20 01:08 dr-jts

I think CoordinateArraySequence has the same issue, due to Coordinates.create(int dimension, int measures) not checking for the case D=4, M=0 here.

dr-jts avatar Sep 01 '20 21:09 dr-jts

I think CoordinateArraySequence has the same issue, due to Coordinates.create(int dimension, int measures) not checking for the case D=4, M=0 here.

Oh yes it seems so :-( I'll have a deeper look tomorrow and try to apply a similar patch with tests. Should I add it to the same branch ?

mukoki avatar Sep 01 '20 22:09 mukoki

I think CoordinateArraySequence has the same issue, due to Coordinates.create(int dimension, int measures) not checking for the case D=4, M=0 here.

Oh yes it seems so :-( I'll have a deeper look tomorrow and try to apply a similar patch with tests. Should I add it to the same branch ?

I merged the PR, so not sure if you can add to the same branch. You should probably make a new branch off master.

dr-jts avatar Sep 01 '20 22:09 dr-jts

OK. In CoordinateArraySequence and CoordinateArraySequenceFactory, there are some documentation and tests verifying that dimension is not > 3. Ex. https://github.com/locationtech/jts/blob/4804f760a1e58c1991dddb2967bd7dd0bed423e1/modules/core/src/main/java/org/locationtech/jts/geom/impl/CoordinateArraySequenceFactory.java#L65-L73 Should I remove this limitation and try to make it work in a way that is as close as possible as PackedCoordinateSequence ?

mukoki avatar Sep 02 '20 06:09 mukoki

@mukoki, there is #306 which points out this limitation. At that point in time it was agreed to not change the semantics of this function. I support your request, though.

FObermaier avatar Sep 02 '20 08:09 FObermaier

After reviewing this and #306 again, I'm wondering if it is the right decision to map 4D to XYZM. On one hand, it does seem all that harmful. But on the other hand, the reasons given below I don't think are totally convincing:

Nothing states that fourth dimension of a Coordinate and followings must be measures.

Well, there is the fact that everywhere in the JTS code (and in these discussions) a 4th dimension is always referred to as a measure.

Problem is that if the user build a 4d CoordinateSequence using a constructor with only the dimension parameter, he isn't warned that he will soon loose the fourth dim of his Coordinates

The Javadoc states the semantics. Otherwise how would you "warn" them? Only way I can think of is throwing an exception - which isn't a warning, it's a hard error. We decided that it is better to make this enforced in a softer way. After all, if the user is relying on this behaviour, it will presumably find out elsewhere in the code.

I don't think there's any obligation on the part of JTS to automatically map 4D to XYZM. If XYZM is required, then the client can simply request it by specifying dim=4 and measures=1.

I do agree though that perhaps dimension should have always been limited to 3, with the measure value adding to the number of ordinates. I think... this stuff is crazy confusing.

dr-jts avatar Sep 02 '20 17:09 dr-jts

Hi Martin, Agree that the definition of dimension and measure are confusing.

After all, it seems to me that throwing exception in builders everytime d > 3 (builder with d parameter only) or d > 3+m (builder with d and m parameters) would be safe and OK. If we accept more than 3 spatial dimensions anywhere in a constructor/builder, it is difficult to respect the principle of least surprise

  • converting 4th ordinate to M (as in my last commit about RayCrossingCounter) : ok, but why adding a measure which is not specified by the user ? I somewhat agree with you on that point.
  • clamping to 3d as in CoordinateArraySequenceFactory : well documented but what about 4d coordinates built with PackedCoordinateSequenceFactory, then processed using CoordinateArraySequence. Should we also clamp in PackedCoordinateSequenceFactory ? Or coordinates built with one factory may loose their 4th dimensions if they are processed with CoordinateArraySequence later.
  • handling more than 3 spatial dimensions and preserving all the dimensions along the processing pipeline : maybe nice, but seems more difficult and probably useless for now.

My 2 cents,

mukoki avatar Sep 02 '20 22:09 mukoki

it seems to me that throwing exception in builders everytime d > 3 (builder with d parameter only) or d > 3+m (builder with d and m parameters) would be safe and OK.

We had this behaviour originally, but I think it caused some regressions in client code.

  • converting 4th ordinate to M (as in my last commit about RayCrossingCounter) : ok, but why adding a measure which is not specified by the user ? I somewhat agree with you on that point.

At the moment I'm thinking that the simplest thing to do is to do what you originally suggested - accept dimension=4 and map it to XYZM automatically (since that is the only supported option).

  • clamping to 3d as in CoordinateArraySequenceFactory : well documented but what about 4d coordinates built with PackedCoordinateSequenceFactory, then processed using CoordinateArraySequence. Should we also clamp in PackedCoordinateSequenceFactory ? Or coordinates built with one factory may loose their 4th dimensions if they are processed with CoordinateArraySequence later.

Not sure this is a problem. Code that creates a new CS to handle an existing one should check the dimension and measures and create the new sequence correctly.

Am I right in taking the view that this is all based on a user ignoring the fact that JTS really only supports XYZ and a single measure, and who then requests a 4D CS, with no intention to use the extra ordinate as a measure? This is really out of scope of what JTS is designed to do. So really they have to understand how the library works, and accept the consequences for what they do.

dr-jts avatar Sep 02 '20 23:09 dr-jts

Am I right in taking the view that this is all based on a user ignoring the fact that JTS really only supports XYZ and a single measure, and who then requests a 4D CS, with no intention to use the extra ordinate as a measure?

As I stated in #306 I believe anyone wanting to create a 4D CS without a measure doesn't care how that ordinate is stored internally, he/she will always access that value by using Coordinate's or CoordinateSequence's get|setOrdinate() functions. That fact that you'd get the same value using getM() is a neglectable side effect in this case.

FObermaier avatar Sep 07 '20 06:09 FObermaier

Problem is that if the user build a 4d CoordinateSequence using a constructor with only the dimension parameter, he isn't warned that he will soon loose the fourth dim of his Coordinates as in CoordinateSequence.toCoordinates() (because of PackedCoordinateSequence.getCoordinateInternal() implementation)

NTS deals with that (edit: for CoordinateSequence implementations other than Packed) with our own special subclass ExtraDimensionalCoordinate which we use in our Coordinates.Create.

It lets Coordinate methods support everything that the underlying sequence supports, and the extra array allocation only comes into play when the user is doing something weird, like (dimension = 4, measures = 0) or (dimension > 4).

So could we maybe just have PackedCoordinateSequence use Coordinates.Create and apply the same kind of change to JTS that we did in NTS, where Coordinates.Create(int dimension, int measures) can create a valid instance for anything with measures >= 0 && (dimension - measures) >= 2?

airbreather avatar Sep 18 '20 11:09 airbreather

I see the struggle with the dualities here and I fixed this by allowing the Geometry to have what it has because to its domain it shall be what it is; yet on a Manifold it will respect some Axis:

public enum Axis
    {
        MinValue,
        Dontcare = Dimension.Dontcare,
        None = 0,
        X = 1, //TODO use binary literal
        Horizontal = X,
        Longitudinal = Horizontal,
        Roll = Horizontal,
        Y = 2,//TODO use binary literal
        Vertical = Y,
        Lateral = Vertical,
        Transverse = Lateral,
        Pitch = Transverse,        
        Z = 4, //TODO use binary literal    
        Azimuth = Z,
        Depth = Azimuth,
        Yaw = Depth,
        Heading = Yaw,
        Inverse = Depth,
        W = 8,//TODO use binary literal        
        A = 16,//TODO use binary literal      
        B = 32,//TODO use binary literal        
        C = 64,//TODO use binary literal
        D = 128,//TODO use binary literal
        Spherical = 9,//TODO use binary literal
        Cylindrical = 15,//TODO use binary literal
        Hyperbolic = 31,//TODO use binary literal
        Quantized = Hyperbolic,
        MaxValue
    }

This works out nicely by having the bits packed to indicate the axii discretely from the Dimension which is specific to Geometry then.

I also have IAxis which provides the Axis rather than having the Axis be referenced directly, the added indirection allows many different possibilities for IParabola...

Given all the above you can consider a Shape a subclass of Geometry which has components which are Vectors and can be converted to Coordinates but are not stored as such for performance reasons, therein when you refer to said Dimension present or not you have its value but you also have contract that its coordinates are XYZM, yet the Components are in a different order yet they can be converted to your order when needed through conjugation. Most 2 dimensional shapes can be represented with just a single Vector right? (Width, Height, Radii and Reserved) are the Components you have from the Vectoral the Geometry controls the Space.

I find this rather eloquent personally but more importantly efficient.

juliusfriedman avatar Dec 12 '20 04:12 juliusfriedman