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

Clipper2 offset closed and open polylines together as Clipper1 #707

Closed
olologin opened this issue Nov 10, 2023 · 11 comments
Closed

Clipper2 offset closed and open polylines together as Clipper1 #707

olologin opened this issue Nov 10, 2023 · 11 comments

Comments

@olologin
Copy link

olologin commented Nov 10, 2023

Hi, as we know, Clipper2 offsets open paths differently compared to Clipper1:
https://angusj.com/clipper2/Docs/Units/Clipper.Offset/Classes/ClipperOffset/_Body.htm

When offsetting open paths, the offset delta indicates the width of the open path (line) once it has been offset (inflated).

Feature request: Is it possible to introduce a special flag that makes Clipper2 offset open paths on the same width as Clipper1?
Because currently we cannot offset closed paths on "delta", and open paths on "delta*2" in the same "Execute" step to simulate Clipper1 result.
In a lot of usecases we used Clipper1 on mixed inputs of Open and Closed paths, and now we have to do this offset separately or come up with workarounds to achieve result of Clipper1.

Honestly I don't even understand why this behavior is changed, because every time I heard about offsetting it was defined as minkowski sum of geometry and circle of radius "delta".

@AngusJohnson
Copy link
Owner

I'm prepared to concede that this change may have been a mistake. However, the rationale for this change was premised on the understanding that software developers generally inflate lines to a specified width. So, rather than inflating open paths by half the desired line width, users could inflate lines to the desired width. One way out of this would be to introduce a compiler conditional, but I'm not (yet) persuaded that there are more than a handful of Clipper2 users still struggling to adapt to this change.

@olologin
Copy link
Author

olologin commented Nov 10, 2023

We were thinking we can try to "Close open path with backturn" to simulate old behavior, but it doesnt seem to produce any result. Anyway, it is not a problem, but inability to offset open and closed contours on consistent width is a problem IMO.

From left to right are inputs (yellow) and outputs (red) of a single execute (delta ~ 0.1) on 3 input paths: Open path, Closed path, Open path that was closed with backturn (with 0-area)
image

@olologin
Copy link
Author

olologin commented Nov 10, 2023

but I'm not (yet) persuaded that there are more than a handful of Clipper2 users still struggling to adapt to this change

The issue is not in adapting to this change, the issue is that old result is impossible to achieve now in one "Execute" operation. It seems the only way to achieve old result is to do 2 Executes separately, on open paths with delta and on closed paths delta*2, and later do Boolean OR of both results. But I am not sure if it is equivalent in terms of performance.

And in general I have a feeling that definition of Minkowski sum with a circle is a better rationale than "software developers generally inflate lines to a specified width" :) Old behavior was consistent in this sense.

@AngusJohnson
Copy link
Owner

OK. If I changed this back now, I'm not sure the best way to do so without creating a headache for those who've adapted to Clipper2's current line offsetting behaviour. I'm open to suggestions. 😉

@olologin
Copy link
Author

@AngusJohnson
I think for C++ compile definition that switches to Clipper1 behavior if defined is good enough. If this can be supported by all of your supported programming languages.
Alternatively you can add flag for offsetter.Execute or offsetter.AddPath(s), this should definitely be supported by all languages, and will allow people to switch between behaviors without recompiling. Although I really cannot imagine why such switch might be needed in runtime.

@philstopford
Copy link
Contributor

philstopford commented Nov 11, 2023

The problem with the compile switch is that some folks use nuget or similar to get packages for use in their projects. That is already a problem in case the user wants the Z value (the nuget is compiled with the defaults, where USINGZ is not set). Now, you could have a 4-way variation (USINGZ, single-sided offset; USINGZ double-sided offset; single-sided offset; double-sided offset).

I wondered if it might be cleaner to have an optional argument in the call that configures the call to be 'double-sided' or 'single-sided'. Default would perhaps be to retain compatibility with the existing code. That or a constructor argument that sets the behavior.

AngusJohnson added a commit that referenced this issue Nov 15, 2023
Minor changes to clipper.export.h
  Renamed DisposeExportedCPaths64 to DisposeArray64
  Renamed DisposeExportedCPathsD to DisposeArrayD
  Tidied up code documentation
@philstopford
Copy link
Contributor

Do I correctly understand that c3839ac has opted to break the existing behavior?

@AngusJohnson
Copy link
Owner

AngusJohnson commented Nov 16, 2023

Do I correctly understand that c3839ac has opted to break the existing behavior?

Yes. I took the plunge 😱😜.
I'm hoping 🤞 the commit comment is sufficient warning.

@olologin
Copy link
Author

I am really happy with your decision, but I express my condolences to everyone who was depending on previous behavior of open offset :)

@philstopford
Copy link
Contributor

I'm not entirely looking forward to reacting to this change, having had to accommodate the change in the behavior from Clipper1-Clipper2 back in the day. Given how long the library has had this behavior, a 'legacy' toggle for the old behavior would have been kind to existing users, but I appreciate that may have been unwelcome effort. No hard feelings.

AngusJohnson added a commit that referenced this issue Nov 26, 2023
  Changes since ver 1.2.3 include:
  1. Important: Offsetting open path behaviour has changed.
     The delta now behaves as it did in Clipper1 (See Issue #707)
  2. Important: Data structures of exported values in clipper.export.h have changed.
  3. Minor bugfixes to PolyTree nesting (#679, #687)
  4. Numerous minor bugfixes to polygon offsetting (#703, #715, #724)
  5. Fixed an obscure bug in polygon clipping caused by horizontal spikes (#720)
  6. Significantly improved documentation.
@mbartelsm
Copy link

mbartelsm commented Nov 27, 2023

I just want to point out that a change in behaviour like this is at least deserving of a minor version increase. This is not a bugfix, this is a breaking change.

Something to consider for future releases.

AngusJohnson added a commit that referenced this issue Nov 28, 2023
1. Fixed a significant offsetting bug introduced in previous update (#733)
2. Fixed a C++ compile error when 32bit compiling (#727)
3. Minor tweak to CMakeLists.txt (C++) (#728)
4. Will now offset 'flat' polygons (Disc.#725)
5. Reminder: recent change in open path offsetting behaviour (#707)
6. Reminder: recent changes to clipper.export.h data structures.
jiajuncccc added a commit to jiajuncccc/Clipper2 that referenced this issue Dec 15, 2023
* remotes/origin/main:
  Minor code tidy
  Added CLIPPER2_HI_PRECISION option in CMakeLists.txt
  Additional minor tweaks to cpp/benchmark tests
  Make googletest not be a git submodule, and instead clone it separately in the CI script (AngusJohnson#745)
  fix compiler warning of -Wunused-but-set-parameter with -fno-exceptions (AngusJohnson#744)
  Additional tweaks to GetIntersectPtBenchmark.cpp
  Additional GetIntersectPtBenchmark tweaks
  Fixed incorrect code comment
  Added new GetIntersectPoint benchmark test Updated other benchmark tests.
  Code formatting only (removed trailing spaces)
  Fixed minor typecasting issues (AngusJohnson#727)
  More tweaks to PointInPolygon benchmarking.
  Allow using external gtest with GTest:: qualified names (AngusJohnson#737)
  Fixed a significant (though uncommon) bug in polygon clipping (AngusJohnson#736) Fixed bugs in PointInPolygon benchmark testing.
  Minor tweaks to PointInPolygon benchmark testing.
  Fixed a minor syntax error tripping some C++ compilers
  Version 1.3.0 1. Fixed a significant offsetting bug introduced in previous update (AngusJohnson#733) 2. Fixed a C++ compile error when 32bit compiling (AngusJohnson#727) 3. Minor tweak to CMakeLists.txt (C++) (AngusJohnson#728) 4. Will now offset 'flat' polygons (Disc.AngusJohnson#725) 5. Reminder: recent change in open path offsetting behaviour (AngusJohnson#707) 6. Reminder: recent changes to clipper.export.h data structures.
  Fixed a typo and tweaked documentation
  Version 1.2.4   Changes since ver 1.2.3 include:   1. Important: Offsetting open path behaviour has changed.      The delta now behaves as it did in Clipper1 (See Issue AngusJohnson#707)   2. Important: Data structures of exported values in clipper.export.h have changed.   3. Minor bugfixes to PolyTree nesting (AngusJohnson#679, AngusJohnson#687)   4. Numerous minor bugfixes to polygon offsetting (AngusJohnson#703, AngusJohnson#715, AngusJohnson#724)   5. Fixed an obscure bug in polygon clipping caused by horizontal spikes (AngusJohnson#720)   6. Significantly improved documentation.
  Additional minor bugfix to ClipperOffset (AngusJohnson#724 & Disc.AngusJohnson#726)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants