Core: Fix BitMap RDP algorithm - #94602
Conversation
|
I'll take a look as soon as I'm able, really excited to see a potential fix to this! |
|
Will do some testing with the MRP I made for a previous bug involving this, very useful for these purposes, uploading here for new use: |
|
It seems the assumption that the intersecting segment must always be at the end is not valid, it fails with cases like this one: Some more examples: So while the improvements to the general algorithm are great this does not solve the self intersection problem at least not reliably |
|
Thanks for pointing that out. I am going to continue working on implementing the star RDP algorithm then. |
|
Having worked on trying to make it work myself I know it's a challenging task, might be worth it to split this into two PRs, one doing the basic fixes to the behavior and one to try and fix the self-intersections |
|
I agree. I also believe the ending self-intersection check should be kept as it covers the edge cases from #69725. I'll do another pull request when the star RDP is done. |
|
Do you mean #69725? |
|
yeah |
|
I finally finished the star RDP and am including it in this pull request. AFAIK it handles all self-intersection cases. |
b3af604 to
34459a6
Compare
|
While investigating why the check failed I stumbled upon a bug within the algorithm implementation. While I fixed it, I still need to do more testing. :( |
|
This is something to do extensive testing on indeed, and a big question is how to approach performance, and we need to measure the performance here and weigh the benefits and drawbacks
But that'd be a matter of performance testing on complex cases like large images to confirm that it is indeed not very slow I would suggest an optional argument defaulting to |
34459a6 to
a867b7b
Compare
73ed862 to
6c7ec19
Compare
|
Sorry for the slow update, I haven't had much time to work on this last week. The bug has been fixed, but I am unsure about how to go about making the star RDP optional. If I add a bool, it will force me to always add the default epsilon value every time I call the function and want to use the flag. |
|
What is the harm of leaving star RDP on for all cases? |
|
The cost, since most inputs don't need this, but that would depend on performance measurements as mentioned above |
1693d01 to
7fbc886
Compare
5b90145 to
4f1f430
Compare
| } | ||
|
|
||
| static bool does_obb_collide_with_line(const Vector<Vector2> &obb, const Vector<Vector2> &line) { | ||
| int obb_size = 4; // obb is a rectangle |
There was a problem hiding this comment.
Can generate_obb_from_polyline() return 2 points for a straight line? Is this always 4?
There was a problem hiding this comment.
Yep, it is always 4 points. The function is only called in does_polyline_obbs_collide, which is only called in recursive_obb_collision_check, that has a base_case gate that prevents two lines from entering the function.
There was a problem hiding this comment.
In that case then a DEV_ASSERT might be worth it here, to check the assumptions.
There was a problem hiding this comment.
What about the colinear case, with a three point subchain (0,0), (5,0), (10,0). Can that be a problem?
There was a problem hiding this comment.
Yep, it is always 4 points. The function is only called in does_polyline_obbs_collide, which is only called in recursive_obb_collision_check, that has a base_case gate that prevents two lines from entering the function.
This depends on the caching mechanism and convex hull being bug free, I'm not sure we should rely on this. Can we use obb.size() dynamically inside does_obb_collide_with_line()?
There was a problem hiding this comment.
Yeah, it would be a problem as does_polyline_obbs_collide would call generate_obb_from_polyline, which would crash once the generated convex hull is empty instead of creating the required four points.
A situation where this could happen is after an intersection is fixed and a point is added colinearly between a pair of points.
There was a problem hiding this comment.
Oh srry, your response never loaded until I refreshed, yeah it's the right call to just use obb.size().
| while (iter_node->next()) { | ||
| iter_node = iter_node->next(); | ||
| float ndx = iter_node->get()[0] - iter_node->prev()->get()[0]; | ||
| if (dx * ndx < 0) { // If they are not the same direction |
There was a problem hiding this comment.
| if (dx * ndx < 0) { // If they are not the same direction | |
| if (dx * ndx <= 0) { // If they are not the same direction |
|
Is using a hashmap with first and last point as the key sound? Can there be subchains that have the same start and end but be different within? |
| result, mapped_result, mono_chain_lst, pl); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can this end up inserting the split point into ch1_list instead of ch2_list? Can we guard against this?
There was a problem hiding this comment.
I don't believe it should as on line 971, I have a if statement that switches between ch1_list and ch2_list. Even if the chains are next to each other, find_furthest_perp_point_from_edge will only return a point that was in-between the ch1_list start and end points. If there are no points are in-between, the code will skip the insertion and continue recursing down the rest of the chain.
There was a problem hiding this comment.
I just realized that if there is no new point added, there is no need to recurse down the chain again. Thanks for the heads up.
| new_chain_end_nodes.push_back(chain_node_to_split); // low -> low + 1 | ||
| } else { | ||
| // There are no points that can be added to fix the chain | ||
| new_chain_end_nodes.push_back(chain_node_to_split); |
There was a problem hiding this comment.
| new_chain_end_nodes.push_back(chain_node_to_split); | |
| new_chain_end_nodes.push_back(nullptr); |
There was a problem hiding this comment.
It still needs to be added to the list because col_chain_start and col_chain_end are based off of the index. Chain 1 has index 0 and 1, Chain 2 has index 2 and 3.
|
|
||
| // Returns the index of the point with the furthest perpendicular distance from the edge in the given range | ||
| static int find_furthest_perp_point_from_edge(const PackedVector2Array &pl, const int start, const int end) { | ||
| int max_dist = -1; |
There was a problem hiding this comment.
| int max_dist = -1; | |
| float max_dist = -1; |
It should be a float to correctly store the squared distance.
|
Overall: This is really difficult to implement, so don't be discouraged, as it would be super useful. I would add more defensive programming over assumptions, use Test cases to cover
|
|
I'll make a new pass of this soon! A lot of things got in the way earlier |
There is only one instance of the chain that is actually edited, so if they have the same start the end, the internals should be the same. That is why I went with a list. I also just realized the newly created sub-chains would not have their OBB recalculated to fit the newly added point. This may cause an issue where the newly added point is a spike that creates a new intersection against a chain that wasn't previously colliding. |
|
Thanks for spending the time to review my code! I'll work on it to hopefully get it merged soon. |
|
At a glance this approach looks somewhat familiar to my own attempts to resolve this so it looks like a theoretically sound approach, but will try to dig into the code and theory |
|
Thank you all so much for the work on this! I'm excited to see this fix/feature come together. |
|
Building this PR and will try to do some validation on the cases that was an issue in the bug report, and I'll try to do a pass over the code this week as well, time permitting |
|
Doing some basic testing and have found some cases where I can cause a crash in the PR version, will upload an example for it to replicate 
For ease of replication here's the data itself: It crashes with the following error and trace: Details
So far it does seem to solve at least some of the cases that broke with the pre-existing algorithm, but does seem to have some edge cases where it fails, but I will continue some testing tomorrow probably and look at some of the code Edit: |
Prevent lines from being added to OBB cache. Added guards against colinear points edge case. - Also tries to avoid adding a colinear point on top of the guards. Add quick fix against vertical starting segments. Adjust high_speed_perp_dist to copy. perpendicular_distance 's style. Replace manual binary search with built-in. Avoid recursing down chain if no chain is added. Clear edited chain for recalcing obb.
|
I fixed the edge case that you had mentioned( it was from me directly added all generated OBBs, even the lines, to the cache ). I have also adjusted most of the functions to have defensive programming checks. Co-linear inputs have been protected against and as well as vertical start segments. |
|
I'll take a look either today or next week! |
|
Did some light testing and will test some more next week but did run into one cases that fails with both algorithms: It looks like this 
But I will look more next week was just messing around with it, will see if I can look at some more robust ways to identify different edge cases for it |
10 participants
Fixes BitMap.opaque_to_polygons can still create invalid polygons #69725