Add intersects function

[seadowg]

Closes #6963

This adds the function so it can be used in constraints for geoshape and geotrace questions. There's one deviation from the issue:

If the parameter type is not geotrace or geoshape, an error is thrown

Instead of throwing an error, the function will just return false in this scenario. We can discuss in the issue if that's correct or not, and if I'm wrong follow-up with another PR.

Why is this the best possible solution? Were any other approaches considered?

I chose to add this using JavaRosa's Function plugin system rather than making changes in JavaRosa itself. I'm very in favour for using JavaRosa's plugins as much as possible: it means we limit changes to and the size of JavaRosa itself, allows us to write code in Kotlin, and we don't have to do a JavaRosa release to add a new feature. We could consider moving the other geo specific functions (area, distance and geofence) to Collect as well - I'll let @grzesiek2010 tell me if he thinks I should follow up with that or not.

In terms of the function itself, I had to relearn a bunch of geometry of various levels I'd forgotten to work out how to implement intersection detection as it didn't seem like there was a widely available library I could pull in (outside something like AWT). Checking my thinking here is probably the most important part of review as this is the kind of thing where automated testing can only give us so much confidence.

The solution I ended up with uses what seems like a fairly standard trick in computational geometry where you can determine the "orientation" (or "direction") of a set of points by calculating their cross product. You can then use this orientation to work out whether the endpoints of a line segment are on the either side of another line. There are definitely other solutions to this problem: you can solve it algebraically using linear equations and there's also specific algorithms for finding self intersections in polygons. I wanted to start with something that was hopefully as easy to understand as possible though to someone jumping into the code fresh or after some time. I imagine there are definitely things we can do to optimize even just the approach we have so far (which currently has a worst case of On^2 with respect to checking line segments against others) before looking at more complex solutions, but let's make sure we need to do that before going down that route.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

As I mentioned above, the biggest risk here is that I've messed up the maths of checking how lines intersect so trying to throw both realistic and weird examples at this would be great. I also think trying this on larger traces/shapes would be interesting to get a sense of performance. Other than intersects though, there should be no changes to the app.

Before submitting this PR, please make sure you have:

• added or modified tests for any new or changed behavior
• run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
• added a comment above any new strings describing it for translators
• added any new strings with date formatting to DateFormatsTest
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[seadowg]

I'm in two minds about this being untested. It can be seen as pure "configuration" from one angle, but also if I delete it then intersects won't work. Maybe it's worth replacing one of the IntersectsFunctionHandler tests (probably the detects intersection path) with a feature test?

[grzesiek2010]

Maybe it's worth replacing one of the IntersectsFunctionHandler tests (probably the detects intersection path) with a feature test?

I think it would make sense.

[seadowg]

@grzesiek2010 marking this as a draft while I add the error cases discussed in #6963. It won't have any significant impact, so feel free to keep reviewing if you already started.

[grzesiek2010]

It doesn't need to be nullable?

[seadowg]

I'll clean the signature of this for Kotlin up as part of updating docs and working out what to do with realTime.

[grzesiek2010]

One more test that fails:

    @Test
    fun `opposite direction segments overlapping`() {
        val trace = Trace(listOf(
            Point(0.0, 0.0),
            Point(2.0, 2.0),
            Point(1.0, 1.0),
            Point(0.0, 0.0)
        ))
        assertThat(trace.intersects(), equalTo(true))
    }

[grzesiek2010]

#6967 (comment)

[grzesiek2010]

@seadowg I’ve found one more case that needs to be handled. I don’t think I’ll be able to find more without spending hours on it. However, after discovering so many cases, I’m not fully confident in accepting the implementation as is. I think this is a perfect case for AI. Maybe we could ask it to generate even 1,000 test cases to cover different scenarios and ensure we don’t miss anything. What do you think?

[seadowg]

I don’t think I’ll be able to find more without spending hours on it. However, after discovering so many cases, I’m not fully confident in accepting the implementation as is.

Agreed! I think we've probably hit the limit of how much confidence we can get from this "ping-pong pairing" style back and forth to generate test cases for ourselves. Beyond this, it'll get harder to think of, describe and later understand cases we might want to test.

I think this is a perfect case for AI. Maybe we could ask it to generate even 1,000 test cases to cover different scenarios and ensure we don’t miss anything. What do you think?

I do think using some form of automated test case generation is needed here. Using an LLM to generate cases could work, but it would add a lot of noise to our test code, and we'd either need to validate each one or accept that there is a risk of false positives from LLM misunderstanding or hallucination.

However, there are some non-AI approaches we could use here. My mind immediately jumps to using something like QuickCheck where we generate random inputs (here traces) and then just test that our function behaves correctly with them. An example of this would be testing an isEven helper:

// This function gets passed a random number as many times as we think we need
fun testIsEven(value: Int) {
    if (value % 2 == 0) {
        assertThat(value.isEven(), equalTo(true))
    } else {
        assertThat(value.isEven(), equalTo(false))
    }
}

The problem we have with intersects is that we can't verify that our function behaves correctly without using an existing implementation. I looked this up, and apparently that's reffered to as the "test oracle problem". As an aside, this also make using an LLM to generate test cases hard: we can't validate that the LLM is producing correct tests without already having a correct implementation.

As mentioned in the "test oracle" wikipedia, one approach I have a vague memory of learning about, but haven't really used is (the very academically named) "metamorphic testing". Here you use the QuickCheck approach, but instead of testing the outputs directly, you test modifications to the input/output that you know should hold:

As an example, when testing a booking website, a web search for accommodation in Sydney, Australia, returns 1,671 results; are the results of this search correct and complete? This is a test oracle problem. Based on a metamorphic relation, we may filter the price range or star rating and apply the search again; it should return a subset of the previous results. A violation of this expectation would similarly reveal a failure of the system.

I think this should be a good technique here. We can easily generate (closed and unclosed) random traces and then test these "metamorphic relations":

• The result of intersects should be consistent when reversing (changing the order of the points), rotating or moving the trace
• If intersects is false, adding a segment that intersects should make it false

Let me know if there are other ones you can think of. This is obviously only a "fuzzy" approach, but without actually writing a proof that will always be the case. I think these checks (and the generation code) should be fairly easy to write, so I can have a go today and see how it looks so we can discuss.

I'll first look at solving #6967 (comment) obviously (which I think is just a mistake in how we deal with closed traces), but after that I think we should hand over to QA just to get more people playing with it.

[seadowg]

@grzesiek2010 Here's the metamorphic test I've added. It currently checks that intersection is consistent when reversing/scaling traces and that adding an intersecting segment to a non-intersecting trace causes intersects to become true. We can tidy this up/formalize it a bit, but I wanted to get your thoughts first! Right now, I'm checking 1000 random traces, but this could of course be less or more depending on how much time/power we want to expend.

[seadowg]

You'll also see that I had to add an epsilon to the collinearity check to handle precision errors and that actually got revealed because of this test which was pretty cool. I'm going to involve @getodk/testers now so we can get some more feedback on using this. It'll be interesting to see if our current calculations are actually too precise when playing around in the real world.

[seadowg]

I did have a quick look at using an existing framework (https://github.com/pholser/junit-quickcheck for example), but it felt like we didn't really need anything so heavy yet. Maybe if we end up writing more of these kinds of tests down the line!

[dbemke]

Should we test this PR on all map engines? If yes, could you send us the apk and check if Google maps token work in it?

[seadowg]

Should we test this PR on all map engines? If yes, could you send us the apk and check if Google maps token work in it?

Map engines won't matter so you can just test with OSM. The actual geotrace/shape features are untouched: this just adds a function you can use for constraints/calculations.