EMI: Relax height check for bridges between sectors #1127
Conversation
chkr-private
commented
Dec 12, 2014
- fixes a path finding issue (EMI: No path found for guybrush when getting the boulders on monkey island #992)
Although the new margin of 0.1 fixes #992, I'm not sure how we could verify whether the new value is actually meaningful/correct. |
👍 I have a feeling 0.5 is the right number, but I cannot back it up :) |
If 0.01 works in all but 1 case, and 0.1 fixes that case, is there any advantage of raising it to 0.5? |
Actually, is there a case where we actually need this check at all? Are there any sectors that overlap which aren't supposed to be crossed between? |
I think this was inherited from Grim. I can't remember any places where it would cause harm, but it's been a while since I last played through. |
The height check is there because the intersection tests are done in 2D, so the code may otherwise consider two sectors at different heights as intersecting. This could potentially lead to the character snapping from one floor to another. @tobiaspfaff experienced this in his mouse mod (see #842). I haven't checked how the sectors are arranged in the boulder area, but if I had to guess, it's possible there are two intersecting sectors that are not parallel with each other. The current bridge building algorithm doesn't handle nonparallel sectors correctly. For pairs of nonparallel sectors, the bridge should be the intersection (in 3D) between the sector polygons, which is just a single line segment if the polygons are intersecting at all. If my guess is correct, implementing this may fix #992. |
@Akz-: I have checked the problematic sectors: they are indeed not parallel. Even worse, one sector isn't even a plane. Implementing 3D intersection for non-plane polygons would be complicated. Given the minor benefit of such an implementation, I believe that the best way to proceed would still be just to relax the height check. |
I have a hunch we keep coming back to this, so I'd appreciate a comment with a reference to issues here before merging. |
- fixes a path finding issue (residualvm#992)
0d3bbc9
to
39b0172
Compare
@somaen: I have added some comments to the commit. OK to merge? |
EMI: Relax height check for bridges between sectors