-
Notifications
You must be signed in to change notification settings - Fork 216
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
Subsequent errors in difference related to switch traversal #888
Comments
Excellent. So two additional test cases to make sure regression does not occur? |
Indeed. The related PR #889 can be merged first. Then (or actually I'm starting already locally) I can try to fix that in the branch where I've the traverse fix, and it should (ideally) not regress (if I can manage) |
Take your time. Next week i am away on summer break. I wont be able to test for a while. |
Thanks! It seems to be a one line fix which is already done 🙂 Do you have more testsuites? Can you verify the new behaviour? (no hurry) Have a nice break anyway! |
I think i can try if this works before my summer break. Generating more test cases might be possible. But this will surely be after my summer break. |
If all goes well i can also try to merge back into tilemaker and see if it works well. If i do a planet conversion you really have a test suite of 10.000s of polygons |
That is more than enough! After break is fine of course. For the first set, let me know if it works / if you agree with merging the PR (#887) |
There is one additional case I would like to try before i can answer this question. And this requires me to change some things to my geometry correct. But I think i can manage to do this within coming days. I also would like to check the existing test cases in geometry correct, so, please be patient. |
Sure, all right and I’ll be patient! Thanks for all your efforts! |
I have the following additional test-case which was valid before and generates invalid polygon with #887: I did some more testing, and changed the test-case. The other test-case i found, had also errors in the old boost, but this one only has one invalid sym_difference:
|
Thanks again. I will add this testcase. And I analyzed it already. It is not easy to fix. Not only because it's huge, the specific conditions for this one, why it should be kept isolated and all others not, are not easy to determine. This requires either more conditions or even another approach. Therefore, and also because it already had errors before as you mention, it is not a regression, and I propose to merge the current approach. I will add a specific ticket for this one later. I prefer to first continue with the effort I was working on (finish remove scaling) and have that in next Boost version, then concentrate on this complex and (I think) uncommon case. @awulkiew @vissarion agree? If yes please approve #887 (Adam did already) and I will merge it and create a small PR for this testcase only after that. |
Are you sure? Because this is indeed an uncommon case. But the case which triggered this bug report was also uncommon. and the behaviour of previous version was working well in many cases it seems. |
My reasoning is that the nearly trivial case (which Adam extracted) might be quite common and should be supported. But revising traversal/switch might take me several weeks again (I work only ~4 hours/week on this project) and we really want to release the version in 1.78 where rescaling is removed (that is my main goal for already more than a year). Therefore I consider that more important than fixing this huge, complex, difficult and uncommon case... (though I keep it on my list and want to fix that afterwards, of course) |
This is a follow up of issue #869 and the pull request #887
@kleunen found an issue with this code (I adapted it slightly to give more output):
The result is (both with
BOOST_GEOMETRY_NO_ROBUSTNESS
and without), in the develop branch:Is valid: true
but in the branch where it is fixed:
First set of input causing invalid output:
Second set of input:
The text was updated successfully, but these errors were encountered: