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

Fix segment intersection consistency in Geometry2D #52110

Merged

Conversation

pouleyKetchoupp
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp commented Aug 25, 2021

Segment collision results could be different depending on the direction when they exactly touch (order of the points in segments). This was due to the way parallelism was checked, using different logic based on positive or negative sign of cross product.

Now the results are the same whatever the direction, without changing the current design, which is that parallel or colinear segments are not considered colinear.
It's still probably safer to make this change only on master to avoid unexpected side effects.

Fixes inconsistencies with raycasts exactly on edges of convex shapes depending on the direction.
Fixes #52088
Edit: fixes #41011 as well


Apart from the physics issue, here's an example that shows inconsistencies.

Script (just needs a Control node to run):

extends Control

var A = Vector2(0, 0)
var B = Vector2(0, 50)
var C = Vector2(50, 0)

var offset = Vector2.ZERO

var font : Font = get_theme_font("Label")

func _draw():
	test_segment(A, B, A, C)
	test_segment(B, A, A, C)

func test_segment(from_a, to_a, from_b, to_b):
	var text = "%s%s / %s%s" % [from_a, to_a, from_b, to_b]
	draw_string(font, offset + Vector2(0, -20), text)
	
	from_a = from_a + offset
	to_a = to_a + offset
	from_b = from_b + offset
	to_b = to_b + offset
	
	var result = Geometry2D.segment_intersects_segment(from_a, to_a, from_b, to_b)
	if result:
		draw_circle(result, 5, Color.YELLOW)
	
	draw_arrow(from_a, to_a, Color.GREEN)
	draw_arrow(from_b, to_b, Color.BLUE)
	
	offset = offset + Vector2(0, 100)

func draw_arrow(from, to, color):
	draw_line(from, to, color)
	
	var dir = (to - from).normalized()
	var tangent = dir.orthogonal()
	
	draw_line(to, to + tangent * 3 - dir * 5, color)
	draw_line(to, to - tangent * 3 - dir * 5, color)

Result (segments in green and blue, shows a yellow dot when there is an intersection):
image

With this PR, both of these cases detect the intersection the same way:
image

@pouleyKetchoupp
Copy link
Contributor Author

Checks are currently failing on this unit test:

 tests/test_geometry_2d.h:143: ERROR: CHECK_FALSE( Geometry2D::segment_intersects_segment(Vector2(-1, 1), Vector2(1, -1), Vector2(0, 1), Vector2(1, -1), &r) ) is NOT correct!
  values: CHECK_FALSE( true )
  logged: Parallel segments should not intersect.

It says Parallel segments should not intersect but the segments used in the test are not parallel so I'm not sure what the intention was.
@mbrlabs Do you remember if this test is covering a specific use case the engine relies on? I see some discussions in #44274 about is_point_in_* functions, but not about the segment/segment intersection.
Otherwise I'll probably just change this test and add some new ones to cover the cases this PR is fixing.

@mbrlabs
Copy link
Contributor

mbrlabs commented Aug 25, 2021

That must have been a mistake of mine...at least the error message as they are cleary not parallel and touch in (1, -1). Maybe i tried to test that touching != intersecting at the time but i don't remember.

You can replace the points in the test with Vector2(-1, 1), Vector2(1, -1), Vector2(0, 1), Vector2(2, -1). These are parallel :)

@pouleyKetchoupp pouleyKetchoupp force-pushed the fix-segment-intersection branch from 509137a to fe61132 Compare August 25, 2021 23:21
@pouleyKetchoupp pouleyKetchoupp requested a review from a team as a code owner August 25, 2021 23:21
@pouleyKetchoupp
Copy link
Contributor Author

pouleyKetchoupp commented Aug 25, 2021

@mbrlabs Thanks! I've updated the parallel segment test following your suggestion and added new ones for touching segments.

Edit: Also as a note, is_point_in_polygon now returns true on the edge of the polygon, which makes it consistent with is_point_in_circle.

@pouleyKetchoupp pouleyKetchoupp force-pushed the fix-segment-intersection branch 3 times, most recently from d916dde to 3282d06 Compare August 26, 2021 01:09
Segment collision results could be different depending on the direction
when they exactly touch (order of the points in segments). This was due
to the way parallelism was checked, using different logic based on
positive or negative sign of cross products.

Now the results are the same whatever the direction, without changing
the current design, which is that parallel or colinear segments are
not considered colinear.

Fixes inconsistencies with raycasts exactly on edges of convex shapes
depending on the direction.
@pouleyKetchoupp pouleyKetchoupp force-pushed the fix-segment-intersection branch from 3282d06 to 511c80b Compare August 26, 2021 01:18
@Xrayez
Copy link
Contributor

Xrayez commented Aug 26, 2021

Could this fix #41011 as well?

It's still probably safer to make this change only on master to avoid unexpected side effects.

True.

@pouleyKetchoupp
Copy link
Contributor Author

Yes, it will fix #41011 as well. Thanks! I've added it to the description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants