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

Allow configuring of the i_overlay Rayon transitive dep. Document more features. #1250

Merged
merged 4 commits into from
Nov 1, 2024
Merged

Allow configuring of the i_overlay Rayon transitive dep. Document more features. #1250

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e5b719
Allow configuring of the `i_overlay` dep, Rayon transitive dep, and d…
frewsxcv Nov 1, 2024
6e23039
Update Cargo.toml
frewsxcv Nov 1, 2024
c85ed96
apply changes
frewsxcv Nov 1, 2024
3c62ec1
merge from origin/main
frewsxcv Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ jobs:
uses: actions/checkout@v3
- run: cargo check --all-targets --no-default-features
# we don't want to test `proj-network` because it only enables the `proj` feature
- run: cargo test --features "use-proj use-serde"
- run: cargo test --features "use-proj use-serde i_overlay earcutr multithreading"

geo_traits:
name: geo-traits
Expand Down
5 changes: 3 additions & 2 deletions geo/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ rust-version = "1.75"
categories = ["science::geo"]

[features]
default = ["earcutr", "spade"]
default = ["earcutr", "spade", "i_overlay", "multithreading"]
use-proj = ["proj"]
proj-network = ["use-proj", "proj/network"]
use-serde = ["serde", "geo-types/serde"]
multithreading = ["i_overlay/allow_multithreading"]
Copy link
Member

Choose a reason for hiding this comment

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

This multithreading feature also enables i_overlay - is that intended?

If so, then I think we should call it something like i_overlay_multithreading (or per my other comment, bool_ops_multithreading)

Though I could imagine a use case for a multithreading flag in geo more generally. In which case this name is good as is! But we should change the definition to:

multithreading = ["i_overlay?/allow_multithreading"]

Which enables allow_multithreading on i_overlay if we're using i_overlay, but otherwise don't enable i_overlay just because someone tried to build geo with multithreading.

Copy link
Member

@michaelkirk michaelkirk Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My own preference is:
multithreading = ["i_overlay?/allow_multithreading"]

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change it! I didn't know about this syntax


[dependencies]
earcutr = { version = "0.4.2", optional = true }
Expand All @@ -30,7 +31,7 @@ proj = { version = "0.27.0", optional = true }
robust = "1.1.0"
rstar = "0.12.0"
serde = { version = "1.0", optional = true, features = ["derive"] }
i_overlay = "1.7.2"
i_overlay = { version = "1.7.2", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

IMO boolean operations are such a core feature that I wouldn't oppose i_overlay itself being required. But I think the multithreading should be optional.

Also, don't you want default-features = false here, so that the i_overlay/allow_multithreading feature here is only applied when multithreading is on?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes great catch, I meant to add default-features = false here

Copy link
Member Author

Choose a reason for hiding this comment

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

6e23039

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO boolean operations are such a core feature that I wouldn't oppose i_overlay itself being required. But I think the multithreading should be optional.

I agree boolean operations are important, but the success of geo up until the operations were added to the crate indicates many people don't need that functionality and the dependency tree that comes with it. So they should have the ability to opt-out. Adding the extra cfg check here is low maintenance cost for us

Copy link
Member

@michaelkirk michaelkirk Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just to be clear, we've had non optional boolean operations in geo for years now. So if anyone is building geo with no-default features, this will be a breaking change for them.

I personally have a slight preference for "make rayon optional but bool_ops mandatory" - because of the above breakage and also the additional configuration and documentation complexity outweighs the benefits of letting people fine tune, in this case, in my estimation.

But it's only a slight preference.

Question: if we break the no-default-features build, do we need to make another bump to 0.X.0?

Copy link
Member

Choose a reason for hiding this comment

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

if anyone is building geo with no-default features, this will be a breaking change for them.

That hadn't occurred to me. I'm now also in the "slight preference for mandatory" camp.

Copy link
Member Author

Choose a reason for hiding this comment

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

I still have a preference for making the bool ops stuff optional but it sounds like there's agreement about making it required so I'll just change it


[dev-dependencies]
approx = ">= 0.4.0, < 0.6.0"
Expand Down
2 changes: 2 additions & 0 deletions geo/src/algorithm/bool_ops/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ use crate::geometry::{LineString, MultiLineString, MultiPolygon, Polygon};

/// Boolean Operations on geometry.
///
/// Requires the `"i_overlay"` feature, which is enabled by default.
///
/// Boolean operations are set operations on geometries considered as a subset
/// of the 2-D plane. The operations supported are: intersection, union, xor or
/// symmetric difference, and set-difference on pairs of 2-D geometries and
Expand Down
8 changes: 7 additions & 1 deletion geo/src/algorithm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@ pub use kernels::{Kernel, Orientation};
pub mod area;
pub use area::Area;

/// Boolean Ops such as union, xor, difference;
/// Boolean Ops such as union, xor, difference.
///
/// Requires the `"i_overlay"` feature, which is enabled by default.
#[cfg(feature = "i_overlay")]
pub mod bool_ops;
#[cfg(feature = "i_overlay")]
pub use bool_ops::{BooleanOps, OpType};

/// Calculate the bounding rectangle of a `Geometry`.
Expand Down Expand Up @@ -254,6 +258,8 @@ pub mod translate;
pub use translate::Translate;

/// Triangulate polygons using an [ear-cutting algorithm](https://www.geometrictools.com/Documentation/TriangulationByEarClipping.pdf).
///
/// Requires the `"earcutr"` feature.
#[cfg(feature = "earcutr")]
pub mod triangulate_earcut;
#[cfg(feature = "earcutr")]
Expand Down
2 changes: 2 additions & 0 deletions geo/src/algorithm/triangulate_earcut.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::{coord, CoordFloat, CoordsIter, Polygon, Triangle};

/// Triangulate polygons using an [ear-cutting algorithm](https://www.geometrictools.com/Documentation/TriangulationByEarClipping.pdf).
///
/// Requires the `"earcutr"` feature, which is enabled by default.
pub trait TriangulateEarcut<T: CoordFloat> {
/// # Examples
///
Expand Down
26 changes: 21 additions & 5 deletions geo/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
//!
//! ## Boolean Operations
//!
//! - **[`BooleanOps`]**: combine or split (Multi)Polygons using intersecton, union, xor, or difference operations
//! - **[`BooleanOps`]**: combine or split (Multi)Polygons using intersecton, union, xor, or difference operations. Requires the `"i_overlay"` feature, which is enabled by default.
//!
//! ## Outlier Detection
//!
Expand Down Expand Up @@ -112,7 +112,7 @@
//!
//! ## Triangulation
//!
//! - **[`TriangulateEarcut`](triangulate_earcut)**: Triangulate polygons using the earcut algorithm (requires the `earcutr` feature).
//! - **[`TriangulateEarcut`](triangulate_earcut)**: Triangulate polygons using the earcut algorithm. Requires the `"earcutr"` feature, which is enabled by default.
//!
//! ## Winding
//!
Expand Down Expand Up @@ -177,9 +177,25 @@
//!
//! The following optional [Cargo features] are available:
//!
//! - `proj-network`: Enables [network grid] support for the [`proj` crate]. After enabling this feature, [further configuration][proj crate file download] is required to use the network grid
//! - `use-proj`: Enables coordinate conversion and transformation of `Point` geometries using the [`proj` crate]
//! - `use-serde`: Allows geometry types to be serialized and deserialized with [Serde]
//! - `i_overlay`:
//! - Enables the `i_overlay` crate, which provides boolean operations on geometries.
Copy link
Member

Choose a reason for hiding this comment

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

Can we call this feature bool_ops? I think it will be more meaningful to our users.

Copy link
Member

Choose a reason for hiding this comment

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

Assuming we won't have a separate boolean operations implementation in the future that doesn't depend on i_overlay

Copy link
Member

Choose a reason for hiding this comment

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

Given how long it's taken to get to this point, that seems unlikely. Not impossible, but a small risk IMO.

Copy link
Member

Choose a reason for hiding this comment

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

In some hypothetical world where we get a second boolops implementation, we'd probably be revisiting the feature flags anyway.

Copy link
Member

Choose a reason for hiding this comment

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

In case my original concern wasn't clear: I think a user perusing the feature flags will more quickly understand what a bool_ops feature does, vs. an i_overlay feature.

The fact that we're using i_overlay for bool_ops is an implementation detail that the user probably doesn't care about.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool I'll change it to bool_ops

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, I'm just going to remove the feature

//! - ☑ Enabled by default.
//! - `earcutr`:
//! - Enables the `earcutr` crate, which provides triangulation of polygons using the earcut algorithm.
//! - ☑ Enabled by default.
//! - `proj-network`:
//! - Enables [network grid] support for the [`proj` crate].
//! - After enabling this feature, [further configuration][proj crate file download] is required to use the network grid.
//! - ☐ Disabled by default.
//! - `use-proj`:
//! - Enables coordinate conversion and transformation of `Point` geometries using the [`proj` crate]
//! - ☐ Disabled by default.
//! - `use-serde`:
//! - Allows geometry types to be serialized and deserialized with [Serde].
//! - ☐ Disabled by default.
//! - `multithreading`:
//! - Enables multithreading support for the `i_overlay` crate.
//! - ☑ Enabled by default.
//!
//! # Ecosystem
//!
Expand Down
Loading