Surface_sweep_2: Handle new overlap on the right of an event that is interior to a curve #7243
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…urves on the right
|
@efifogel could you add your minimal case in the testsuite? |
|
The geometric tests indeed seem awkward.
I will look into it.
____ _ ____ _
/_____/_) o /__________ __ //
(____ ( ( ( (_/ (_/-(-'_(/
_/
…On Fri, 3 Feb 2023 at 12:20, Sebastien Loriot ***@***.***> wrote:
Handle case of overlapping curve starting on a non start/end event.
@efifogel <https://github.com/efifogel> I don't like the fact that I'm
adding some geometric tests. I think I should check if there are
overlapping curves on the right of the event and check if the originating
curves are in the status line (using the stored hint). What do you think?
Fixes #7235 <#7235>
------------------------------
You can view, comment on, or merge this pull request online at:
#7243
Commit Summary
- 2260c4f
<2260c4f>
more than one curve can be on the left of an event with overlapping curves
on the right
File Changes
(1 file <https://github.com/CGAL/cgal/pull/7243/files>)
- *M*
Surface_sweep_2/include/CGAL/Surface_sweep_2/Surface_sweep_2_impl.h
<https://github.com/CGAL/cgal/pull/7243/files#diff-0d2b6f8ea7e75b3324fac9cc7ff1d3a2285a25436e79066002fb7af49bba5757>
(19)
Patch Links:
- https://github.com/CGAL/cgal/pull/7243.patch
- https://github.com/CGAL/cgal/pull/7243.diff
—
Reply to this email directly, view it on GitHub
<#7243>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVBNOEDHARHZNFXDTDHUDDWVTLXXANCNFSM6AAAAAAUQCVUHU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I will extract the input polylines that cause the issue and use them for
the minimal tests.
____ _ ____ _
/_____/_) o /__________ __ //
(____ ( ( ( (_/ (_/-(-'_(/
_/
…On Fri, 3 Feb 2023 at 12:21, Sebastien Loriot ***@***.***> wrote:
@efifogel <https://github.com/efifogel> could you add your minimal case
in the testsuite?
—
Reply to this email directly, view it on GitHub
<#7243 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVBNODUA4DBQVTFMETUR6DWVTLZPANCNFSM6AAAAAAUQCVUHU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
lrineau
changed the title
…apping curves on the right" This reverts commit 2260c4f.
|
@efifogel 0714c96 is actually the real fix for this case. What happens is the following: We are at event A, which is also the left event of curve c1. During handle_left_curve, event A is deleted. Then in handle_right_curve, we have c1 and c2 that are adjacent and that create overlapping curve c3. That curve starts at event B. BUT event B is allocated at the same address as event A, which later prevents c1 and c2 to be added as left curves of event B. Resetting left event of curves after deletion solves the pb. I wonder if the fix is at the right place and if we shouldn't do it also for non-overlapping and no intersection sweep versions. Note that |
|
Let me try to fix them quickly.
…On Fri, Dec 15, 2023, 10:37 Sebastien Loriot ***@***.***> wrote:
@efifogel <https://github.com/efifogel> there is actually a conflict with
the update of tests with #6721 <#6721>,
which should actually be responsible for the Conic failure mentioned in
your observer PR. What do you recommend? undo the changes related to conics
in this PR?
—
Reply to this email directly, view it on GitHub
<#7243 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVBNOHFRGTGEODLFYYE5OTYJQD43AVCNFSM6AAAAAAUQCVUHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJXGQ4DOMJQGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Note that this branch targets 5.5.x and the problem is when we merge the branch with master (that targets 6.0) |
|
I see. Then in this case perhaps it's better to split.
I've forked from your branch and created the branch 'mine
Ss_2-test_conics-efif', where I merged with master and fixed the conic
tests.
Feel free to remove it from your branch. When the time is right, I'll
create a pull request for this new one.
____ _ ____ _
/_____/_) o /__________ __ //
(____ ( ( ( (_/ (_/-(-'_(/
_/
…On Fri, 15 Dec 2023 at 11:13, Sebastien Loriot ***@***.***> wrote:
Note that this branch targets 5.5.x and the problem is when we merge the
branch with master (that targets 6.0)
—
Reply to this email directly, view it on GitHub
<#7243 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVBNOD5AF4QA3AMYWD3BKTYJQICNAVCNFSM6AAAAAAUQCVUHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJXGUZTINZRGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Successfully tested in CGAL-6.0-Ic-134 |
|
@efifogel You can now submit the PR for the improvement of the test for the conic traits (over 5.6.x AFAIR) |
|
Just did
____ _ ____ _
/_____/_) o /__________ __ //
(____ ( ( ( (_/ (_/-(-'_(/
_/
…On Fri, 22 Dec 2023 at 18:12, Sebastien Loriot ***@***.***> wrote:
@efifogel <https://github.com/efifogel> You can now submit the PR for the
improvement of the test for the conic traits (over 5.6.x AFAIR)
—
Reply to this email directly, view it on GitHub
<#7243 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVBNOGZMNOTJGTU7IJB6ITYKWWQHAVCNFSM6AAAAAAUQCVUHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRXHA2TOMRUGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Handle case of overlapping curve starting on a non start/end event.
@efifogel I don't like the fact that I'm adding some geometric tests. I think I should check if there are overlapping curves on the right of the event and check if the originating curves are in the status line (using the stored hint). What do you think?
Fixes #7235