Port GenericTests from Rust lib

[stefannikolei]

This is just a rough fix for the last test failure of the js generic tests where we have a mismatch in the count of points.

This is also done in the rust version. mod.rs L:116

Do you see a better way how to handle this? Perhaps change the return value of ConnectEdge to Contour[] and then do the ordering?

When this is done only 2 tests of the js test suite are failing. And those faile because of probably some rounding problems

[JimBobSquarePants]

@stefannikolei I'll have a look at this asap. I've been preoccupied looking at some gnarly ImageSharp issues.

As a side, I've been porting more of the Rust version code into a local branch. There's a bit of difference in that codebase around segment comparing plus some intersection calculation changes. It doesn't fix things yet but it may make a good starting point to getting everything working so will push the branch tomorrow.

[stefannikolei]

@JimBobSquarePants I looked further into the remaining failing tests. I get a difference in the connectEdge logic. Our implementation for example return 8 as the nextPos. But the Rust implementation returns 9.

The rust implementation is using an precalculated iteration_map. I think our focus should be on this one.

I will wait for your remaining rust ports and then look further into it.

[stefannikolei]

@JimBobSquarePants I added some of your changes from your rust port into this branch. I Got the tests down to 9 failing tests.

I did not add the changes from the SegmentComparer. This made more tests fail.

Will try to debug some smaller tests to hopefully find the remaining problem(s)

The initial JS Code can not help here.
The Testcases which are failing now are also failing for the JS version

[stefannikolei]

@JimBobSquarePants I now have ported the PrecomputeIterationOrder. This brought down the failing tests to 7

[JimBobSquarePants]

I think I was going in the right direction with the segment comparer changes however I must have missed something. I didn't have the chance yet to port the tests for the comparer for the rust version so I'm sure if we do that, we'd soon find my issue.

[stefannikolei]

@JimBobSquarePants I incorporated your changes to the segment comparer. I fixed the Comparer Tests. Now we are down to 6 failing tests

[stefannikolei]

Updated the test cases from the rust implementation. Now we are down to 3 failing tests

[JimBobSquarePants]

Amazing! so close now. Thanks so much for your help!

[stefannikolei]

This fixes Issue76. BUT collapsed_edges_removed failed now. Throwing that on the rust implementation also produces an error.

SO i am unsure what to do..

[stefannikolei]

Perhaps we should make 2 tests.

One that has all the tests from the js version. Other one with the rust ones.

Your goal is to get all green or just js or rust?

[JimBobSquarePants]

I think we should stick with the Rust tests as that port is an evolution of the JS one and appears to be more correct.

[stefannikolei]

Soo I had some time to come back to this. I found some things.

I am looking at daef_holed_rectablge2.json

I enhanced the output of the test a bit

Expected ContourCount: 3
Actual   ContourCount: 3
Contour 0: Expected VertexCount=4, Actual VertexCount=3
    Expected (X, Y)        |    Actual (X, Y)
    (0, 0)               |    (0, 0)              
    (150, 300)           |    (150, 300)          
    (0, 300)             |    (0, 300)            
    (0, 0)               |                        
Contour 1: Expected VertexCount=4, Actual VertexCount=4
    Expected (X, Y)        |    Actual (X, Y)
    (60, 60)             |    (60, 60)            
    (150, 60)            |    (150, 60)           
    (150, 240)           |    (150, 240)          
    (60, 60)             |    (60, 60)            
Contour 2: Expected VertexCount=7, Actual VertexCount=7
    Expected (X, Y)        |    Actual (X, Y)
    (150, 0)             |    (150, 0)            
    (300, 0)             |    (300, 0)            
    (150, 300)           |    (150, 300)          
    (150, 240)           |    (150, 240)          
    (240, 60)            |    (240, 60)           
    (150, 60)            |    (150, 60)           
    (150, 0)             |    (150, 0)

So it seems to see that the first Contour is not closed. Or better said the First and Last point are the same but the last point is omitted

[JimBobSquarePants]

Thanks for looking at this again. I'll see if I can debug the rust version; the number of vertices are small enough that it's feasible.

[stefannikolei]

So I could locate the problematic function. The problems lies in the connect_edges--> order_events.

The Rust version has a list of 24 items. Our C# version produces just 22

Here is the Rust List as reference:

index	x	y	other_x	other_y	lr	result_transition	in_out	other_in_out	is_subject	is_exterior_ring	prev_in_result
0	0.0	0.0	150.0	300.0	L	OutIn	true	false	true	true	None
1	0.0	0.0	0.0	300.0	L	InOut	false	false	true	true	Some("COORD(0.0 0.0)")
2	0.0	300.0	0.0	0.0	R	None	false	false	true	true	None
3	0.0	300.0	150.0	300.0	L	InOut	true	true	false	true	Some("COORD(0.0 0.0)")
4	60.0	60.0	150.0	60.0	L	OutIn	true	false	true	false	None
5	60.0	60.0	150.0	240.0	L	InOut	false	false	true	false	Some("COORD(60.0 60.0)")
6	150.0	0.0	300.0	0.0	L	OutIn	false	true	true	true	None
7	150.0	0.0	150.0	60.0	L	InOut	false	false	false	true	Some("COORD(150.0 0.0)")
8	150.0	60.0	150.0	0.0	R	None	false	false	false	true	None
9	150.0	60.0	60.0	60.0	R	None	false	false	true	true	None
10	150.0	60.0	240.0	60.0	L	InOut	true	true	true	true	Some("COORD(150.0 0.0)")
11	150.0	60.0	150.0	240.0	L	OutIn	false	true	false	true	Some("COORD(150.0 60.0)")
12	150.0	240.0	150.0	60.0	R	None	false	false	false	true	None
13	150.0	240.0	60.0	60.0	R	None	false	false	true	false	None
14	150.0	240.0	240.0	60.0	L	OutIn	false	true	true	false	Some("COORD(150.0 60.0)")
15	150.0	240.0	150.0	300.0	L	InOut	false	false	false	true	Some("COORD(150.0 240.0)")
16	150.0	300.0	150.0	240.0	R	None	false	false	false	true	None
17	150.0	300.0	0.0	0.0	R	None	false	false	true	true	None
18	150.0	300.0	0.0	300.0	R	None	false	false	false	true	None
19	150.0	300.0	300.0	0.0	L	InOut	true	true	true	true	Some("COORD(150.0 240.0)")
20	240.0	60.0	150.0	60.0	R	None	false	false	true	false	None
21	240.0	60.0	150.0	240.0	R	None	false	false	true	false	None
22	300.0	0.0	150.0	0.0	R	None	false	false	true	true	None
23	300.0	0.0	150.0	300.0	R	None	false	false	true	true	None

[JimBobSquarePants]

@stefannikolei I've also been debugging this, and I've identified the first difference is at index 5 in the list. Somehow our item at that index is being set with EdgeType.NonContributing.

I've just pushed some minor changes to your branch that do not affect this result but more closely match the Rust.

[JimBobSquarePants]

My changes broke more tests so I've reverted them

[stefannikolei]

I could look a bit more into it. Could not locate the problem. But the filled event_queue already has differences in it

index	x	y	other_x	other_y	lr	result_transition	in_out	other_in_out	is_subject	is_exterior_ring	edge_type	prev_in_result	contourid
0	0.0	0.0	300.0	0.0	L	None	false	false	true	true	Normal	None	1
1	0.0	0.0	150.0	0.0	L	None	false	false	false	true	Normal	None	2
2	0.0	0.0	150.0	300.0	L	None	false	false	true	true	Normal	None	1
3	0.0	0.0	0.0	300.0	L	None	false	false	true	true	Normal	None	1
4	0.0	300.0	0.0	0.0	R	None	false	false	true	true	Normal	None	1
5	0.0	0.0	0.0	300.0	L	None	false	false	false	true	Normal	None	2
6	0.0	0.0	0.0	300.0	L	None	false	false	true	true	Normal	None	1
7	0.0	300.0	0.0	0.0	R	None	false	false	true	true	Normal	None	1
8	150.0	0.0	0.0	0.0	R	None	false	false	false	true	Normal	None	2
9	150.0	300.0	150.0	0.0	R	None	false	false	false	true	Normal	None	2
10	0.0	300.0	0.0	0.0	R	None	false	false	false	true	Normal	None	2
11	150.0	240.0	60.0	60.0	R	None	false	false	true	false	Normal	None	1
12	150.0	240.0	240.0	60.0	L	None	false	false	true	false	Normal	None	1
13	240.0	60.0	150.0	240.0	R	None	false	false	true	false	Normal	None	1
14	240.0	60.0	60.0	60.0	R	None	false	false	true	false	Normal	None	1
15	300.0	0.0	0.0	0.0	R	None	false	false	true	true	Normal	None	1
16	60.0	60.0	240.0	60.0	L	None	false	false	true	false	Normal	None	1
17	150.0	300.0	0.0	0.0	R	None	false	false	true	true	Normal	None	1
18	150.0	0.0	150.0	300.0	L	None	false	false	false	true	Normal	None	2
19	150.0	300.0	300.0	0.0	L	None	false	false	true	true	Normal	None	1
20	150.0	300.0	0.0	300.0	R	None	false	false	false	true	Normal	None	2
21	60.0	60.0	150.0	240.0	L	None	false	false	true	false	Normal	None	1
22	0.0	300.0	150.0	300.0	L	None	false	false	false	true	Normal	None	2
23	300.0	0.0	150.0	300.0	R	None	false	false	true	true	Normal	None	1

the thing that is really strange is the fact that we get a contourid of 3 and in the rust version doesn't even exist an 3

[JimBobSquarePants]

That’s because we assign an id per contour id per polygon contour but the rust uses a for each on the type assigning only a single id.

I’m stumped as to why it’s different though..

I think what we need to do it to spilt the code into static methods and copy the individual tests from the rust implementation

[stefannikolei]

So i experimented a bit with Copilot and let him write a function for creating an image with ImageSharp.Drawing. So we have a visualisation of the results :)

[stefannikolei]

I had another go on debugging. In my eyes the problem lies somewhere in the subdivide logic.

Probably in the Implementation of the Statusline and the segment comparer.

I am going to look deeper into it by adding debug outputs (there are some in rust but I need to port them to c# and make some adjustments to the rust output to make it easier to find the difference :)

I will keep you posted

[stefannikolei]

I added some outputs.

At some point they start to differ when getting the prevEvent und NextEvent. So I am assuming we have probably a Problem in the SegmentComparer or the StatusLine.

That's not so easy to debug..

[JimBobSquarePants]

Good job narrowing it down. There are comparer tests in the rust code aren’t there? We should replicate them.

[stefannikolei]

Good job narrowing it down. There are comparer tests in the rust code aren’t there? We should replicate them.

I think you have already ported the tests. But you used an SortedSet there and not your StatusLine. That is a bit suspicious for me :)

[JimBobSquarePants]

Haha! Yeah, VERY suspicious!

[stefannikolei]

I ported a test method from the compare_segment class. And I Think I have now found the root of the problem. Debugged it just for a second. Will try to analyse it and port the rust code.

In the rust code they switch the events:

let (se_old_l, se_new_l, less_if) = if se1_l.is_before(se2_l) {
    (se1_l, se2_l, helper::less_if as fn(bool) -> Ordering)
} else {
    (se2_l, se1_l, helper::less_if_inversed as fn(bool) -> Ordering)
};

That's the only thing that I could identify as a difference atm.

[stefannikolei]

So the last failing test issue76 does not have a problem in the Comparer Code.

Rust has a eventQueue filled with 16 and we have only 12 in it. So it happens before the Subdivide_segments

[stefannikolei]

I debugged it a while until I realised that the tests are started with different values oO

Let's say it this way. The Bug is in the test code not in the lib. :)

[stefannikolei]

@JimBobSquarePants 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

[JimBobSquarePants]

Oh you absolute legend!!! Well done! I’ll review this asap.

[stefannikolei]

@JimBobSquarePants I am done :) When this is merged I will push a small reduction in Allocations. Don't wanna push it not to make it bigger. And you reviewed this one already. So doing it in an extra PR

[JimBobSquarePants]

Let’s get this in!!

[stefannikolei]

5 month in the making. Now you can open a good bottle of whisky.

And in total 5 years