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

CoordinateSequence: Add type-detecting forEach (requires C++14) #800

Merged
merged 4 commits into from
Jan 20, 2023

Conversation

dbaston
Copy link
Member

@dbaston dbaston commented Jan 15, 2023

As a follow-on to the introduction of XY, XYM and XYZM Coordinate types to GEOS, I have been exploring the feasibility of supporting these coordinate types in the overlay operations. Consistent with the new CoordinateSequence implementation, a design goal is to avoid a penalty for M values in cases where they are not used. (Good news: it's already working for simple cases such as line intersections).

One problem I have come across several times already is the need to perform some operation to each coordinate in a sequence, in type-generic but type-preserving way. For example, OverlayMixedPoints::findPoints needs to read a CoordinateSequence, find unique coordinates matching a certain condition that only requires use of X and Y, and write them to an output CoordinateSequence.

One way do do this is to copy everything into a CoordinateXYZM when we read it:

set::set<OverlayXY> points;
CoordinateSequence result(0, coords.hasZ(), coords.hasM());
CoordinateXYZM tmp;
for (std::size_t i = 0; i = inputSeq.size(); i++) {
	inputSeq.getAt(i, tmp);
	if (matchesCondition(c) && points.insert(c).second) {
		resultSeq.add(c);
	}
}

Of course, the problem here is that we copy the coordinate every time we handle it, which goes against the goal by introducing a new performance penalty for XY and XYZ operations. If we want to avoid the copy, we need to work with references matching the actual backing type of the CoordianteSequence. The simplest way to do this is by checking the backing type of the CoordinateSequence and providing an implementation for each type, for example:

set::set<OverlayXY> points;
CoordinateSequence result(0, coords.hasZ(), coords.hasM());

switch(getCoordinateType(inputSeq)) {
    case CoordinateType::XY:    for (const auto& c : items<CoordinateXY>())   { if (matchesCondition(c) && points.insert(c).second) resultSeq.add(c); } break;
    case CoordinateType::XYZ:   for (const auto& c : items<Coordinate>())     { if (matchesCondition(c) && points.insert(c).second) resultSeq.add(c); } break;
    case CoordinateType::XYM:   for (const auto& c : items<CoordinateXYM>())  { if (matchesCondition(c) && points.insert(c).second) resultSeq.add(c); } break;
    case CoordinateType::XYZM:  for (const auto& c : items<CoordinateXYZM>()) { if (matchesCondition(c) && points.insert(c).second) resultSeq.add(c); } break;
}

That's simple and also very repetitive. In #799, I proposed a CoordinateInspector base class that lets you provide a template that hooks into the existing CoordinateFilter machinery. So instead of the code above, you could write

struct CheckUniqueAndAdd : public CoordinateInspector<CheckUniqueAndAdd> {
public:
	CheckUniqueAndAdd(const OverlayMixedPoints& p_omp, CoordinateSequence& p_result) : omp(p_omp), result(p_result) {}

	template<typename CoordType>
	void filter(const CoordType* c) {
		if (omp.matchesCondition(c) && points.insert(c).second)
 		  result.add(c);
	}

	OverlayMixedPoints omp;
	set::set<OverlayXY> points;
	CoordinateSequence& result;
}

CoordinateSequence result(0, coords.hasZ(), coords.hasM());
CheckUniqueAndAdd filter(*this, resultSeq);
seq.apply_ro(&filter);

This avoids repeating the implementation, but is harder to read and requires a lot of tedious plumbing. We can't define the template at the spot where it's used, we need to bring in any variables the implementation requires as member variables, write a contructor, etc. A much cleaner solution is available with auto lambdas:

CoordinateSequence result(0, coords.hasZ(), coords.hasM());
set::set<OverlayXY> points;

seq.forEach([this, &points, &result](const auto& c) {
	if (matchesCondition(c) && points.insert(c).second)
	  result.add(c);
});

The problem (?) with the above is that it requires C++14. Switching to C++14 may sound like a disruptive change but in practice should have no effect. C++14 is fully supported by all of our CI environments including gcc 5. C++14 is not supported by gcc 4.8, but GEOS already does not build on gcc 4.8 since #726.

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.

None yet

1 participant