Skip to content

feat: added support for GeometryCollections#1384

Open
shaietz wants to merge 5 commits intoopenlayers:mainfrom
shaietz:main
Open

feat: added support for GeometryCollections#1384
shaietz wants to merge 5 commits intoopenlayers:mainfrom
shaietz:main

Conversation

@shaietz
Copy link
Copy Markdown

@shaietz shaietz commented Dec 9, 2025

Issue #1383 add support for GeometryCollection Feature type

modified stylefunction to handle GeometryCollections by calling the stylefunction on each geometry individually

Copy link
Copy Markdown
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this, @shaietz. The starting point would be to create a geojson with valid GeometryCollections, preferably one with two simple geometries and one with a nested geometry collection and a simple geometry.

A test would also be crucial to make problems with the added code obvious.

I added comments for each of the problems I see from looking at the code. Please address them. Let me know when you have questions.

}
}

return styles.length > 0 ? styles : undefined;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work. Styles are reused across styleFunction() calls, so you need to clone them, and each style needs to be configured with the geometry of the geometry collection.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure i understand what you mean, anyways i have removed the push of the substyle to the styles array because it was not necessary, so is this still relevant?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, what I wrote is still relevant. Also regarding a test case with nested geometry collections.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi, sorry for the long break, i ended up not needing this feature which is the reason i opened the issue in the first place, however i have some free time now and want to finish this up.
regarding this:

This won't work. Styles are reused across styleFunction() calls, so you need to clone them, and each style needs to be configured with the geometry of the geometry collection.

i don't understand why this won't work and what needs to be done in order for it to work, since i call the styleFunction() recursively for each sub geometry shouldn't it handle handle adding the individual styles to the styles array correctly?

also, i added a nested geometry collection to the geojson-inline.json and it seems to be working correctly, should i also add a test to stylefunction.test.js?

thanks for your patience

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why it appears to work is because OpenLayers takes care of correct rendering of all points of the GeometryCollection, and the last style will be used. With geometry collections with mixed types, that won't work so well.

Honestly, I cannot think of a way to deal with this properly. Maybe taking a look at how MapLibre deals with GeometryCollection geometries from GeoJSON sources could make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants