Remove irrelevant points for view

[gluser1357]

Pull request for enhancement request https://trac.osgeo.org/postgis/ticket/5705
See details there, thanks.

[robe2]

@gluser1357 Looks like it's failing on our bots treating warnings as errors. Can you fix:

https://github.com/postgis/postgis/actions/runs/8474473891/job/23259878295?pr=767

lwgeom_remove_irrelevant_points_for_view.c: In function 'ST_RemoveIrrelevantPointsForView':
lwgeom_remove_irrelevant_points_for_view.c:192:20: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  192 |         for (i=0; i<mline->ngeoms; i++) {
      |                    ^
lwgeom_remove_irrelevant_points_for_view.c:212:28: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  212 |                 for (i=0; i<polygon->nrings; i++) {
      |                            ^
lwgeom_remove_irrelevant_points_for_view.c:223:48: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  223 |                                     for (k=0; k<polygon->nrings; k++) {
      |                                                ^
lwgeom_remove_irrelevant_points_for_view.c:241:28: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  241 |                 for (j=0; j<mpolygon->ngeoms; j++) {
      |                            ^
lwgeom_remove_irrelevant_points_for_view.c:245:32: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  245 |                     for (i=0; i<polygon->nrings; i++) {
      |                                ^
lwgeom_remove_irrelevant_points_for_view.c:256:60: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  256 |                                                 for (k=0; k<polygon->nrings; k++) {
      |                                                            ^
cc1: all warnings being treated as errors

[Komzpa]

It looks like this function is actually stepping on the toes of three other things in PostGIS:

1. There is ST_AsMVTGeom that does this clipping using Wagyu library, also making sure it will be valid. My understanding is that any tile-related renderer should just reuse ST_AsMVTGeom as most of the painful stuff is figured out there.

2. There is ST_ClipByBox2D which exists exactly for the same goal, to clip geometries for rendering. Original ticket compares performance to ST_Intersection, was ST_ClipByBox2D even considered?

3. There is already a fast range-clipper for those who don't care about validity in PostGIS codebase, in lwgeom_clip_to_ordinate_range. MVT generation utilizes that:

   postgis/deps/wagyu/lwgeom_wagyu.cpp

   Line 67 in ebad0ba

   LWCOLLECTION *geom_clipped_x = lwgeom_clip_to_ordinate_range((LWGEOM *)geom, 'X', box->xmin, box->xmax, 0);

Which of these should we combine together so that there aren't too many copies of the code that does pretty much the same thing? I have a feeling that it might be that it's enough to maybe add more docs to ST_Intersection to link to ST_ClipByBox2D and maybe expose alternative range clipper into ST_ClipByBox2D as some argument. What do you think?

[gluser1357]

Looks like it's failing on our bots treating warnings as errors. Can you fix:
https://github.com/postgis/postgis/actions/runs/8474473891/job/23259878295?pr=767

I changed variables from signed to unsigned int but I'm not sure if this fixes the problem. Can I start the build myself, or how can I test it the same way as your test (treat warnings as errors)?

[gluser1357]

Thank you for the feedback!

I see that there are three similar functions, and honestly didn't know about ST_ClipByBox2D. Thank you for the hint. Linking this in the ST_Intersection docs would be great :) I'll test performance and result for our use case next week.

The both other functions ST_AsMVTGeom and lwgeom_clip_to_ordinate_range I also didn't tested so far. The reason is that our use case is not related to MVT and I'm not sure about the correct parameters for our use case. We also don't want to reduce resolution (I assume that's what the bounds, extent and buffer parameter are introduced for but perhaps I'm wrong with that?) and we don't need to clip exactly (because this is what already the rendering client does). Perhaps someone could assist me on how to choose approriate parameters for ST_AsMVTGeom for our use case and what steps are required to internally use lwgeom_clip_to_ordinate_range?

One advantage of the proposed function over the existing ones above might be that no new coordinates are created and no rounding while computing intersection points has to be applied.

For keeping validity of polygons I have one more idea that I'd like to test next week, too. It may involve adding the coordinates given by the bbox corners.

maybe expose alternative range clipper into ST_ClipByBox2D as some argument.

This sounds interesting. Perhaps it is better to add one or two optional parameters to ST_Intersection instead of introducing a new function. But I'm not sure with that?

[robe2]

Looks like it's failing on our bots treating warnings as errors. Can you fix:
https://github.com/postgis/postgis/actions/runs/8474473891/job/23259878295?pr=767

I changed variables from signed to unsigned int but I'm not sure if this fixes the problem. Can I start the build myself, or how can I test it the same way as your test (treat warnings as errors)?

That fixed the signed warning issue so we are set for this.

[gluser1357]

@Komzpa: I checked ST_ClipByBox2D - the purpose seems to be indeed very similar.

Differences are (see updated docs and illustration at https://github.com/gluser1357/postgis-fork/blob/remove_irrelevant_points_for_view/doc/html/images/static/st_removeirrelevantpointsforview.png):

• no intersection points are computed, avoiding rounding errors and usually increasing performance (according to my tests, in most cases about 10-30% faster compared to ST_ClipByBox2D)
• returns a geometry with a equal or smaller point number compared to ST_ClipByBox2D

The visual result within the specified view is the same, as well as the fact that polygons may be invalid afterwards because of self-intersections (outside the view area). I have an idea how to solve this by applying cut-offs on all four edges of the view bbox and could try to add this if there is some interest in that.

The other functions you metioned, ST_AsMVTGeom and lwgeom_clip_to_ordinate_range, does likely the same as ST_ClipByBox2D but are not (directly) usable. At least I don't know exactly how I would to that for my case. Thus, I'm skipping further evaluation here for now.

Further reviews are welcome :-)

[pramsey]

One of the reasons ST_ClipByBox2D is not really used anymore is because it would produce invalid output for some cases, which many MVT renderers very unhappy with. This resulted in a lot of fiddling around and eventually our adoption of the wagyu clipper for generating MVT geometry.

[gluser1357]

One of the reasons ST_ClipByBox2D is not really used anymore is because it would produce invalid output for some cases, which many MVT renderers very unhappy with. This resulted in a lot of fiddling around and eventually our adoption of the wagyu clipper for generating MVT geometry.

Thank you for your feedback. I have two questions:

1. Are there definitions of polygons and bounds where ST_ClipByBox2D would return invalid output? Although I'm personally happy with the current result of ST_RemoveIrrelevantPointsForView I could try to add code to fix invalid results in a second pass.
2. How can ST_AsMVTGeom (or other MVT functions) be used to process a geometry (with the aim to remove unnecessary points, possibly clipped) without changing the rendering result within the specified bounds? Could you please give an example?

[robe2]

1. Are there definitions of polygons and bounds where ST_ClipByBox2D would return invalid output? Although I'm personally happy with the current result of ST_RemoveIrrelevantPointsForView I could try to add code to fix invalid results in a second pass.

As I recall yes ST_ClipByBox2D can create invalid output. It's focus is on speed rather than validaty.

@pramsey @Komzpa Any thoughts to @gluser1357 question. I haven't used ST_AsMVT much directly.

[gluser1357]

With my latest commit I added some more checks to decrease the resulting point number or even completely empty the result if possible.

Some more notes:

• In terms of the nature of the function there are cases where points can be removed but requires self-intersections at the same time. I added an example in the docs to make that clearer (see also here for a quick view: https://github.com/gluser1357/postgis-fork/blob/remove_irrelevant_points_for_view/doc/html/images/static/st_removeirrelevantpointsforview_crossing.png).
• There might be rare cases where this function would result slightly more points compared to ST_ClipByBox2D (that's in contrast to what I wrote in my comment two weeks ago). The new example above is such one.
• The algorithm doesn't require allocs. It works sequentially on groups of three consecutive points as explained in the code.
• There are some known cases where a minor improvement (slightly less points in the result) could be achieved by checking which point(s) of a sequence of outside points would be optimal to keep. Since this would introduce a lot more code complexity and a backing array and would likely have no real practical impact this step is skipped.

Thank you for further reviews, remaining in hope that it'll make it into 3.5.0 :)

[gluser1357]

For the sake of completeness I copy the review comments from the original Trac Ticket https://trac.osgeo.org/postgis/ticket/5705 by mdavis in here:

Is this guaranteed to maintain polygonal validity? This should be stated in the docs

Currently, this function does not ensure polygonial validity. I added a warning in the docs (reference_editor.xml). Should I add it also at another location?

I suggest a slightly simpler function name, and one that uses terminology already in use. Perhaps ST_RemovePointsOutsideBBox?

I was really looking for another function name but I couldn't find a better one so far describing what the function does. Since some points are kept outside the box that are required to render the same result within the bbox your suggestion doesn't exactly hit the functionality. What about ST_SimplifyForView, ST_SimplifyForBBox, ST_ReducePointsForView or ST_ReducePointsForBbox?

Alternatively, ST_ClipByBox2D could also be reused with an optional parameter, specifiying if new points may be computed in order to clip to exact bounds or not, e. g.

• ST_ClipByBox2D(geometry geom, box2d box, boolean allow_new_points = true) or
• ST_ClipByBox2D(geometry geom, box2d box, boolean exact = true)

What dou you think?

@Komzpa: Want to add coderabbitai like in #768 here as well? :-)

[robe2]

Does anyone else have anything to say before I commit this? I think it's been here long enough.
If I don't hear anymore in the next couple of hours, I'm going to go ahead and commit.

[robe2]

@coderabbitai review

[coderabbitai]

Walkthrough

Walkthrough

The recent updates introduce a new function, ST_RemoveIrrelevantPointsForView, aimed at optimizing rendering by removing unnecessary points from geometries within specified bounds. This function is incorporated into the codebase, tested, and documented, enhancing performance and rendering accuracy for (MULTI)POLYGON and (MULTI)LINESTRING geometries.

Changes

File(s)	Change Summary
doc/reference_editor.xml	Added documentation for the new function ST_RemoveIrrelevantPointsForView.
postgis/Makefile.in	Added lwgeom_remove_irrelevant_points_for_view.o to PG_OBJS list.
postgis/lwgeom_remove_irrelevant_points_for_view.c	Introduced functions to remove irrelevant points for rendering within specified bounds.
postgis/postgis.sql.in	Added SQL definition for the new function ST_RemoveIrrelevantPointsForView.
regress/core/remove_irrelevant_points_for_view.sql	Added SQL queries for testing ST_RemoveIrrelevantPointsForView.
regress/core/remove_irrelevant_points_for_view_expected	Added expected results for the new test cases.
regress/core/tests.mk.in	Added new test for remove_irrelevant_points_for_view.

Poem

In the land of code where points abound,
A new function was joyfully found,
To trim the excess, keep the view clear,
Rendering shapes with precision near.
Points irrelevant, now swept away,
For a smoother, brighter display!
Hopping through the code, a rabbit's cheer,
For changes that bring efficiency here! 🐇✨

---

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 8c30a25 and de36d4f.

Files ignored due to path filters (2)

• doc/html/images/static/st_removeirrelevantpointsforview.png is excluded by !**/*.png
• doc/html/images/static/st_removeirrelevantpointsforview_crossing.png is excluded by !**/*.png

Files selected for processing (7)

• doc/reference_editor.xml (1 hunks)
• postgis/Makefile.in (1 hunks)
• postgis/lwgeom_remove_irrelevant_points_for_view.c (1 hunks)
• postgis/postgis.sql.in (1 hunks)
• regress/core/remove_irrelevant_points_for_view.sql (1 hunks)
• regress/core/remove_irrelevant_points_for_view_expected (1 hunks)
• regress/core/tests.mk.in (1 hunks)

Files skipped from review due to trivial changes (2)

• regress/core/remove_irrelevant_points_for_view.sql
• regress/core/remove_irrelevant_points_for_view_expected

Additional comments not posted (4)

regress/core/tests.mk.in (1)

98-98: The addition of the remove_irrelevant_points_for_view test is correctly placed in the list of tests.

postgis/Makefile.in (1)

134-134: The addition of the lwgeom_remove_irrelevant_points_for_view.o object file is correctly integrated into the build process.

postgis/lwgeom_remove_irrelevant_points_for_view.c (1)

320-489: The implementation of ST_RemoveIrrelevantPointsForView is robust and well-documented. It correctly handles different geometry types and optimizes the point array based on the specified bounds.

doc/reference_editor.xml (1)

1748-1857: The documentation for the new function ST_RemoveIrrelevantPointsForView is comprehensive and well-structured. It clearly explains the purpose, usage, and behavior of the function, including potential issues with invalid geometries and self-intersections. The examples provided are practical and help illustrate the function's effects. This addition should be very useful for users needing to optimize rendering geometries within specific bounds.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 1

[coderabbitai]

Consider changing the volatility category from VOLATILE to IMMUTABLE.

If ST_RemoveIrrelevantPointsForView does not modify the database state and always returns the same result for the same inputs, it should be marked as IMMUTABLE instead of VOLATILE. This can improve query performance as IMMUTABLE functions allow results to be cached.

[robe2]

I agree 👍 @gluser1357 This should be marked IMMUTABLE, but I can take care of it if you don't get this in time.

[coderabbitai]

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

[gluser1357]

Thank you for merging :)