ISE: Internal failure, constraint not added about tinfour HOT 19 CLOSED

ISE confirmed - no VertexMergerGroup found for near vertices.

I have temporary workaround. I set merge group ID to vertex when adding to group. Before calling processConstraint I replace all vertices in constraint with VertexMergerGroup if applicable. And then I remove all subsequent duplicates.

I still have to make minimal test.

from tinfour.

from tinfour.

`
private static final String[] DATA = {
"0;172.55020519171376;863.1997703686357",
"20;182.60318452026695;883.3712838017382",
"21;182.87705280618334;882.7087832991416",
"22;184.52793324903155;878.7152244266567",
"23;188.2311372301774;869.7569966036826",
"24;211.21082526503596;815.4266806989908",
"25;211.21082526585087;815.4266806961969",
"26;258.02551045722794;704.7449661702849", // comment this line to get NPE from another place
"31;172.55020519171376;863.1997703686357"
};

public static void main(final String[] args) {
    PolygonConstraint constraint = new PolygonConstraint();
    for (String text : DATA) {
        String[] values = text.split(";");
        Vertex vertex = new Vertex(Double.parseDouble(values[1]), Double.parseDouble(values[2]), 1.0);
        constraint.add(vertex);
    }
    constraint.complete();

    double nominalPointSpacing = 10.0;
    IIncrementalTin tin = new SemiVirtualIncrementalTin(nominalPointSpacing);
    tin.addConstraints(Collections.singletonList(constraint), false);
}

`

from tinfour.

Rows with ID 24, 25 are similar. These should fall into same vertex group.

When you comment ID 26. You get NPE from another part of code.
According to JTS polygon constructed from above coordinates is valid for both cases.

from tinfour.

NPE is tracked as #17 NPE: processing polygon constraint

from tinfour.

Here data are valid. No segment intersections, ...

from tinfour.

from tinfour.

from tinfour.

from tinfour.

from tinfour.

from tinfour.

Also, I am still looking at the constraint-member flag. That operation is not correct yet. It is close. But not complete.

from tinfour.

I believe that the constraint member flag is now populated correctly when a merger group is constructed. I also followed your suggestion for the addVertex() logic. If a non-synthetic vertex is added to a merger group that has a synthetic status, the group is set to non-synthetic. This is a bit of a tricky issue, but it appears to be supported by the addConstraints() method.

from tinfour.

Here are my proposed fix from July 7th. Modified is only SemiVirtualIncrementalTIN. I introduced mergeID to Vertex. mergeID is index to coincidenceList and is set when vertex is added to vertex group. After adding all constraints merged vertices are replaced directly with its group in constraint vertex list. Another change is that I introduced TIN_MEMBER flag. It was discussed earlier and refused by you.
tinfour-fix-jandam.zip

from tinfour.

from tinfour.

I looked at the code. I am trying to figure out the best solution based on your needs. I have a question about the TIN_MEMBER flag. But I'd like to talk about the constraint geometry first.

Your solution updates the geometry of the constraint objects that were passed in to the TIN. My version tries to leave the constraint objects unchanged. The reason behind this was that I thought it possible that an application might use the constraints in multiple TINs or in parallel threads. To make it work, I create a separate list of constraint objects internal to the TIN object. Some of these objects may be the original ones that you passed in, some may be "copy objects" with the geometry updated to include the merge vertices. So if you want the true list of constraints that are stored in the TIN, you have to call the getConstraints() method. I'll have to make a note about this in the Javadoc.

Also, I've avoided using an additional mergeID field to the Vertex class. I am reluctant to do so because the field would increase the size of the vertex object by 8 bytes (4 for the integer size, 4 for Java memory padding). But that leads to a question. Is there a reason that you would need the mergeID for your own application?

Also, in the discussion before, I talked about how the TIN_MEMBER flag might have problems because the TIN class would probably be better if it did not alter vertex objects when they are added to the TIN. Also, a single vertex can be added to multiple tins. So while the "add" methods for the TIN classes could set a TIN_MEMBER flag, the TIN classes can never reliably clear them (if, for example, a TIN object goes out-of-scope).

That leads me to another question. How do you foresee your application using a TIN_MEMBER flag?

If you tell me about your anticipated use for the TIN_MEMBER flag, and I can think of a way of supporting your function without interfering with other Tinfour actions, I'll introduce it to the code base. If I can't make that work, you can always create your own derived classes from Vertex and SemiVirtualIncrementalTin to manage it. VertexJanda could use the Vertex.reserved2 field to store information. Once you added vertices and constraints to a TIN, you could call the getVertices() method, get all the vertices in the TIN, loop through them and set the flags. Alternately, you could derive a new SemiVirtualIncrementalTinJanda class that overrides the add() and remove(), and clear() methods to manage the TIN membership flags. But I think the getVertices() method is probably safer.

Gary

from tinfour.

from tinfour.

Dealing with concurrency issues is a challenge in any computer language. In fact, I think its the hardest thing we programmer's do.

One technique for simplifying concurrency issues in Java is to make all the elements in an object final. That way, an application can pass the objects around at will without ever worrying about concurrent-modification or synchronization overhead. Unfortunately, I didn't follow this guideline in the Vertex class. The status flag is modified when the vertex is added to a constraint. The index field is modified by the HilbertSort (I tried really hard to avoid that one, but couldn't think of any solution that didn't involve the need for me to write my own sort routine).

Anyway, I'm thinking now that the code that allows the status field to be modified was a mistake. I should have made it final and had it be set as part of the constructor (getting rid of the setConstraintMember() method). I looked through the Tinfour code and see that nothing in the package itself that would be broken if I did that.

Would that negatively affect your code?

The "reserved" fields would still remain mutable with the assumption that the application code would manage any concurrency issues as necessary for its own purposes.

Unfortunately, I don't think there's much I can do about the index unless I completely refactor the HilbertSort.

from tinfour.

The problematic merge-group issues have been corrected. Therefore I am closing this issue.

If you would like to discuss the additional issues that were raised in this series of comments, please feel free to open a new issue.

Thank you for identifying this problem and helping to improve the Tinfour code.

from tinfour.