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.

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

GEOSClipByRect: preserve Z coordinate #1095

Merged
merged 1 commit into from
May 23, 2024

Conversation

vadimcn
Copy link
Contributor

@vadimcn vadimcn commented May 14, 2024

For [multi]linestrings/linerings, Z coordinate is interpolated from that of the nearest existing vertices.

For [multi]polygons, Z coordinate of points created on the boundary is interpolated as above. For interior points, however, it is populated using the ElevationModel.

@vadimcn vadimcn force-pushed the fix-GEOSClipByRect branch from 29ee395 to e083ae6 Compare May 14, 2024 23:06
@dbaston dbaston added the Enhancement New feature or feature improvement. label May 15, 2024
@vadimcn vadimcn force-pushed the fix-GEOSClipByRect branch from e083ae6 to fdee5a7 Compare May 15, 2024 01:45
For [multi]linestrings/linerings, Z coordinate is interpolated
from that of the nearest existing vertices.

For [multi]polygons, Z coordinate of points created on the boundary
is interpolated as above.  For interior points, however, it is populated
using the ElevationModel.
@vadimcn vadimcn force-pushed the fix-GEOSClipByRect branch from fdee5a7 to a73946f Compare May 15, 2024 01:48
@vadimcn
Copy link
Contributor Author

vadimcn commented May 18, 2024

@dbaston does this look okay to merge now?

@dbaston
Copy link
Member

dbaston commented May 21, 2024

Do you think it makes sense to modify the tests to remove the expected value and just check that the result matches GEOSIntersection ?

    void
    checkClipByRectIdentical(
        const char* wktInput,
        double xmin, double ymin,
        double xmax, double ymax)
    {
        input_ = fromWKT(wktInput);
        result_ = GEOSClipByRect(input_, xmin, ymin, xmax, ymax);


        GEOSGeometry* rect = GEOSGeom_createRectangle(xmin, ymin, xmax, ymax);
        expected_ = GEOSIntersection(input_, rect);
        GEOSGeom_destroy(rect);

        GEOSNormalize(result_);
        GEOSNormalize(expected_);

        bool equal =  GEOSEqualsIdentical(result_, expected_) == 1;
        if (!equal) {
            wkt_ = GEOSWKTWriter_write(wktw_, expected_);
            std::printf("EXP: %s\n", wkt_);
            GEOSFree(wkt_);
            wkt_ = GEOSWKTWriter_write(wktw_, result_);
            std::printf("OBT: %s\n", wkt_);
        }
        ensure(equal);
    }

@vadimcn
Copy link
Contributor Author

vadimcn commented May 21, 2024

I don't know enough about the GEOS project to have a strong opinion on this.
Do you postulate that the result of GEOSClipByRect should always be the same as GEOSIntersection with a box polygon? Or should it be the same after normalization, as you have above? Then maybe yes.

However, currently, several existing tests fail this comparison, see below. I'd say that fixing those is out of scope of this PR.

EXP: POINT EMPTY
OBT: GEOMETRYCOLLECTION EMPTY
[1=F]

EXP: POINT (15 10)
OBT: GEOMETRYCOLLECTION EMPTY
[3=F]

EXP: LINESTRING EMPTY
OBT: GEOMETRYCOLLECTION EMPTY
[4=F]

EXP: MULTILINESTRING ((10 10, 15 10), (10 10, 10 15))
OBT: GEOMETRYCOLLECTION EMPTY
[6=F]

EXP: MULTILINESTRING ((20 10, 20 20), (10 20, 20 20), (10 10, 20 10), (10 10, 10 20))
OBT: GEOMETRYCOLLECTION EMPTY
[10=F]

EXP: MULTILINESTRING ((20 10, 20 20), (10 20, 20 20), (10 10, 20 10), (10 10, 10 20))
OBT: GEOMETRYCOLLECTION EMPTY
[11=F]

@dbaston
Copy link
Member

dbaston commented May 23, 2024

However, currently, several existing tests fail this comparison, see below.

Odd, they are passing for me after normalization. Either way, it can be beyond the scope of this PR.

@dbaston dbaston merged commit a13c2aa into libgeos:main May 23, 2024
30 checks passed
@vadimcn
Copy link
Contributor Author

vadimcn commented May 23, 2024

Odd, they are passing for me after normalization. Either way, it can be beyond the scope of this PR.

I thought the plan was to update all tests, including those still using checkClipByRect. If you only changed checkClipByRectIdentical, I guess it'd work.

@dbaston
Copy link
Member

dbaston commented May 24, 2024

Looking more closely at the examples you pointed out, I can see that the semantics are different from GEOSIntersection. I guess the tests should be left as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or feature improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants