Triggers: Re-expose radius

[Raffson]

A conversation was started in #243 explaining why this PR is needed.

In short, we need the radius in the base-class TriggerZone in order to serialize the mission correctly. The second commit adds a polymorphic method is_point_in_zone that returns True if a given point is inside the zone, False otherwise.

For a circular trigger zone this is pretty straight forward, but for quad-points that was a little trickier. I think I provided enough comments in the code to explain what's happening, but feel free to point out if something else is worth mentioning.

[Raffson]

Changing this to a draft as I want to expand the test to include edge-cases. I'm also doubting whether I made a mistake in determining the constant in case of an "inversion". I'll be revisiting this tomorrow probably...

[Raffson]

@DanAlbert @bobmoretti,

I think I have it locked down, though I could use an extra set of eyes on the re-exposure of the radius property to make sure I haven't missed anything.

As for the is_point_in_zone method, I suggest reviewing the test instead, unless you feel comfortable dealing with mirrored axes compared with DCS' ME, inversions of first order functions (which causes mirroring yet again) and shifting the frame of reference. So yeah, it can become a mind-fuck at some point. Considering the fact that my test passes, which tests 2 cases, I'm confident this approach is viable.

Either way, I think it's worth considering expanding the test to include even more shapes for testing, preferably in the weirdest forms. 😂

[Raffson]

Here's a couple of fun examples that still fail 🤣

[bobmoretti]

We chatted about this offline, but I'll post a summary of my current thinking on this one...

First of all, agreed and aligned on adding back the radius attribute at the TriggerZone base class level.

On the point-in-polygon check, I have some thoughts.

From a cohesion and separation-of-concerns perspective, it's strange to me to include all of this algorithmic code inside what is otherwise a thin layer on top of the DCS miz file responsible for serializing and deserializing trigger zones.

At the very least the algorithmic code doesn't belong in this same file; it should be factored out and called from here.

Also, it seems a bit strange that PyDCS would try to implement such an algorithm in the first place. There are existing Python libraries that can already do this check accurately and quickly and have been thoroughly tested. See for example https://www.matecdev.com/posts/point-in-polygon.html#how-to-check-if-a-point-is-inside-a-polygon-in-python. If users of PyDCS need this functionality, could they just use that instead?

[Raffson]

I've got the algorithm locked down, tests also pass for those weird shapes, and I've included another edge-case where you'd want a trigger "as a line", quite literally. Not sure why anyone would want to use such a construct, but if it's possible in DCS, support in pydcs only seems logical...

As for whether this code is sitting in the right place, I tend to agree that this violates SRP. However, I'm still not sure where to put it instead. Would it sit better in mapping.Point? The reasoning behind that one is the fact that there's already functionality over there that allows for calculating stuff between points like distances, headings, midpoints, etc.
So perhaps we should change the roles, i.e. restructure the code so we can use point.in_zone(triggerzone) instead. To me that sounds reasonable, but I'd like to hear other opinions/solutions too.

[rp-]

Ehm, can't we use the Polygon.point_in_poly ?
https://github.com/pydcs/dcs/blob/master/dcs/mapping.py#L297

I mean isn't a QuadPoint not just a Polygon with 4 points?

[Raffson]

Ehm, can't we use the Polygon.point_in_poly ?

https://github.com/pydcs/dcs/blob/master/dcs/mapping.py#L297

I mean isn't a QuadPoint not just a Polygon with 4 points?

I stumbled upon this rather late, but looking at the algorithm I have my doubts about its correctness. I'll try to run my test on it tomorrow to see if my suspicion is justified.

[Raffson]

@rp- I suspected your algorithm would fail with my funny shapes, but the tests actually pass except for 1.

The algorithm fails when the the quad-point's vertices are placed in such a manner that you end up with a line. I know, it's a very unusual/special case which is probably never going to be used, but nonetheless it seems wrong to leave that edge-case "un-fixed".

I also wrote another test to see if the algorithm is really waterproof. I used the following shape:

So here's what I think, I'll simply detach the 2nd commit from this PR so the exposure can be merged already, since that's something we've all agreed upon as being necessary.

As for a more elegant way to test quad-point trigger zones in Liberation, here's the issues we need to deal with:

• Currently we need to check if a given trigger zone is an instance of TriggerZoneQuadPoint
• A polygon requires a terrain in its constructor, that limits the number of places where we can use it to call the 'point_in_polygon' method.
• Polymorphism is no longer an option to solve the first issue, since we haven't got a terrain at hand. That would mean we'd have to provide it as a parameter to the polymorphic method, which pretty much puts us in the same boat as the second issue.

So yeah, any suggestions? 😂

[DanAlbert]

So yeah, any suggestions?

Skip it. Let callers delegate to shapely if they need this.

[Raffson]

So yeah, any suggestions?

Skip it. Let callers delegate to shapely if they need this.

Wilco, stand-by for a new push, though I did include the test for the shape...

[DanAlbert]

Wow, wtf github. Will revert...

[Raffson]

Wow, wtf github. Will revert...

WTF just happened??? 😂

[DanAlbert]

Okay, reverted the thing that wasn't ready to merge yet (github wasn't showing me the failed checked or your comment when I clicked that button, and it looked right). Am afk for a few hours, so will look later.

[Raffson]

Okay, reverted the thing that wasn't ready to merge yet (github wasn't showing me the failed checked or your comment when I clicked that button, and it looked right). Am afk for a few hours, so will look later.

Correct, I forgot to drop the Tuple in the import. And while at it, an extra test never hurts.

Anyway, let me reopen a PR real quick...

[Raffson]

Skip it. Let callers delegate to shapely if they need this.

Wait, why shapely? Seems like I failed to mention 'point_in_polygon' passes the test for the last shape. Imo it's actually a nice algorithm, both efficient and correct, so I'm not sure why you suggest using a different package...

There's multiple ways to omit the 3 problems I mentioned, the question is which one's the neatest.

[DanAlbert]

I thought the question was "how do I implement concave hull collision checking?", and the best answer to "how do I implement solved problem X?" is usually "let someone else do it"