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.

Sign up for GitHub

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

Jump to bottom

ISE: Internal failure, constraint not added #15

Closed
jandam opened this issue Jun 26, 2017 · 19 comments
Closed

ISE: Internal failure, constraint not added #15

jandam opened this issue Jun 26, 2017 · 19 comments

Comments

@jandam
Copy link

jandam commented Jun 26, 2017

*) List of polygon constraints in meters. nominalPointSpacing = 10.0m
*) 1 constraint has 2 vertices at index 78 and 79 with distance 1e-9. They are correctly merged in VertexMergerGroup when adding as vertex.
*) during processConstraint VertexMergerGroup is not recognized for vertex at in 78 and 79.
*) processConstraint throws ISE.
https://github.com/gwlucastrig/Tinfour/blob/master/src/main/java/tinfour/semivirtual/SemiVirtualIncrementalTin.java#L1979

Everything works correctly when I process failing polygon constrain separately.

I have to prepare minimal test case.
@jandam
Copy link
Author

jandam commented Jun 26, 2017

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.

@gwlucastrig
Copy link
Owner

gwlucastrig commented Jun 26, 2017 via email

@jandam
Copy link
Author

jandam commented Jun 27, 2017

`
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);
}

`

@jandam
Copy link
Author

jandam commented Jun 27, 2017

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.

@jandam jandam mentioned this issue Jun 27, 2017
Closed
@jandam
Copy link
Author

jandam commented Jun 27, 2017

NPE is tracked as #17 NPE: processing polygon constraint

@jandam
Copy link
Author

jandam commented Sep 24, 2017

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

@gwlucastrig
Copy link
Owner

gwlucastrig commented Sep 24, 2017 via email

@gwlucastrig
Copy link
Owner

gwlucastrig commented Sep 24, 2017 via email

@jandam
Copy link
Author

jandam commented Sep 24, 2017 via email

@jandam
Copy link
Author

jandam commented Sep 24, 2017 via email •
edited

gwlucastrig added a commit that referenced this issue Sep 25, 2017
@gwlucastrig
Fix for constraints issue #15
6c70a99
@gwlucastrig
Copy link
Owner

gwlucastrig commented Sep 25, 2017 via email

@gwlucastrig
Copy link
Owner

gwlucastrig commented Sep 25, 2017 •
edited

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

@gwlucastrig
Copy link
Owner

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.

@jandam
Copy link
Author

jandam commented Sep 25, 2017

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

@gwlucastrig
Copy link
Owner

gwlucastrig commented Sep 25, 2017 via email

@gwlucastrig
Copy link
Owner

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

@jandam
Copy link
Author

jandam commented Sep 25, 2017 via email

@gwlucastrig
Copy link
Owner

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.

@gwlucastrig
Copy link
Owner

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.

gwlucastrig added a commit that referenced this issue May 6, 2021
@gwlucastrig
Fix for constraints issue #15
af2ac9a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jandam @gwlucastrig