-
Notifications
You must be signed in to change notification settings - Fork 15
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 Test project from NetTopologySuite.IO.ShapeFile repository #7
Conversation
Exclude all files causing compile erros.
Add tests from NetTopologySuite.IO.ShapeFile repository
NetTopologySuite.IO.Esri currently read all polygon shapefiles always as MultiPolygon what is consistent with ESRI Shapefile Technical Description. The same for lines - they are always read as MultiLineString. |
I have some compilation issues in Test.cs, PrintFieldValues should be something like
or am I missing something? There is also something in Beside this, all tests are basically green. |
Thanks for the feedback, @DGuidi . I've fixed compilation errors in the TestConsole app. By the way I also upgraded it from .NET Framework into .NET 6. Assert statement was set for this piece of code var shell = Factory.CreateLinearRing(partsBuilder[0]);
if (shell.IsCCW)
{
if (partsBuilder.Count > 1)
{
ThrowInvalidRecordException("Invalid Shapefile polygon - shell coordinates are not in in clockwise order.");
}
Debug.Assert(!shell.IsCCW, "Invalid Shapefile polygon - shell coordinates are not in in clockwise order.");
shell = Factory.CreateLinearRing(partsBuilder[0].Reversed());
} According to SHP specification vertices for a polygon shell should have clockwise order. I could throw an error if that's not the case, but in order to pass other tests forgiving approach was used. If, contrary to documentation, shell |
I think that this can be related to IO.ShapeFile issue 70 and related PR 80? |
Yes, it's related to issue 70 and PR 80. Currently it works like this:
Above approach was chosen in order to make the reading fast. Personally I don't like the idea of checking every geometry in the world because there can be someone who provided us invalid Shapefile. But if you like to sacrifice performance for convenience I can change the code to follow @FObermaier proposal:
What is the decision? |
In the meantime I've moved all ported test into Depraced folder/namespace. I've also added a new test checking if MultiPolygons that have a polygon inside the hole are handled properly. |
My2cents: as in old library (if I remember correctly), enable the "slower code that checks every geometry" only if a specific flag is enabled. Otherwise assume the data provided is correct and use the faster.code. |
I like this idea. I will add a flag to ShapefileReaderOptions and create two
|
There should be similar flags in the codebase of the io.shapefile library. You can take a look for inspiration, but feel free to choose whatever you prefer. |
I'm going to handle it using following flags (common for all geometry types):
|
@FObermaier @airbreather sorry to bother you. any advice on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the continued work on this.
Add Test project from NetTopologySuite.IO.ShapeFile repository
TODO: Changed original test logic
comments.TODO: Remove no longer relevant test
comments.