ST_AddIsoNode allows adding non-isolated nodes (nodes with containing_face=null)

[strk]

According to SQL/MM, the ST_AddIsoNode function allows passing 'null' as the "containing face". This is allowed specifically for the case in which an isolated node is not inside a face.

Now, since the same document also refers to an "universal face", which has (again, in the spec) a special face identifier 0, I guess that's the value we should write in "containing_face" field of the node table, to distinguish the node from a non-isolated one.

This would be pretty much equivalent to forbid passin 'null' and allow passing '0', but the specs are confusing on the matter.

[aperi2007]

I agree with this solution. Because reading the specs, other funzions work like this.

For example, the ST_GetFaceGeometry. From ISO specs: Allow to pass the 0 value but intercept it giving an "Error - sql/mm spatial exception - universal face has no geometry".

Always the ST_GetFaceGeometry from ISO specs: with a parameter idface = NULL shuold intercept it giving an error: "Error - sql/mm spatial exception - null argument".

So I guess is correct to think to idface=0 as the value for an UniversalFace?, and don't use the NULL value as UniversalFace?.

[strk]

So, then I think we should: 1. Forbid passing NULL as the containing_face 1. If 0 is passed as the containing_face, check that no face contains the node

Agreed?

[aperi2007]

Yes.

Also I guess we could apply a real simplification to workflow because to do this test implicitly we find the face that really contain it :) So even if the user pass idface=0, if the system see that the face is not the UniverseFace? (id=0) but instead the (for example) idface=7. It should put that value instead of 0 instead of raise an error.

[strk]

uhm.. finding the _actual_ face containing it is what Scarponcini suggested as a possible change for handling the 'null' case. I dont' see it being appropriate when the caller explictly sets containment within a face as getting that wrong from the client side might reveal a bug in the user process, so we do want to notify that.

Generally speaking there are a few other places where the ISO mandates might be more comfortable, and this is also true for simple geometries. I belive in such cases we should eventually make our own versions as friendly as they can be and keep the standard names for standard behaviors.

[aperi2007]

Replying to strk:

uhm.. finding the _actual_ face containing it is what Scarponcini suggested as a possible change for handling the 'null' case. I dont' see it being appropriate when the caller explictly sets containment within a face as getting that wrong from the client side might reveal a bug in the user process, so we do want to notify that.

I agree with you. So I prefer use the IDFace value = 0 instead of NULL value. Infact when the user send a parametr idface=0 the system MUST to find the correct face to understand if the UniverseFace? e' correct or not.

Instead if the user give an explicitly value (ie idface=7) and it is wrong the ST_AddIsoEdge should raise an exception.

I should add this option only because the work to find the right face when the user use UniverseFace? ad input parameter is needed to do to understand if it is in the Universeface. (my patch for ST_AddIsoNode work in this mode)

Generally speaking there are a few other places where the ISO mandates might be more comfortable, and this is also true for simple geometries. I belive in such cases we should eventually make our own versions as friendly as they can be and keep the standard names for standard behaviors.

So you guess to raise an exception if the user say to use the IDFace=0 and the code see that relly the isolated node is contained in another face (ie idface=7) ?

OK, I have no problem to agree with this. Even if this mean to add an isolated node always need to do the test 2 times. The first time for find the face that containing the new node (the user want add) for this the user should use the GetFaceByPoint? for example. The second time the test will be do in the ST_AddIsoNode to check if the face is the right face.

[strk]

You could avoid the double check by implementing the semantic Scarponcini suggested when 'null' is passed as the containing_face, and always passing 'null', but then again you wouldn't know (at the client side) which face your node was in, which would speed up next call (if you're adding multiple nodes you know all being within the same face.

I suggest the next step here would be providing a comprehensive regression test for ST_AddIsoNode. Some of them could be extrapolated from the current test/regress/sqlmm.sql file (which is expecting the current behavior, of course...)

[strk]

A patch for this was attached to another ticket:

http://trac.osgeo.org/postgis/attachment/ticket/978/patch_ST_AddIsoNode.zip

[strk]

The patch, slighly modified, was committed as r7222.

I noticed that the test is building its own topology, and filling up MBR and using hexwkb to build it. It'd be easier to maintain if you used the existing reference 'city_data' topology, as done by other recent tests. The reference topology does have faces with holes and w/out holes, isolated edges and pre-existing isolated nodes.