Menu ▾ ▴

#192 Clipper2 Core translated to C++ (results/proposal)

*
open
nobody
Clipper2 (4) C++ (4)
1
2023-05-04
2019-03-10
Andrii Doroshenko
No

Despite the fact that Clipper2 is still being worked on, I've translated some ClipperCore.pas structures/routines to C++. Current implementation uses templates as the library seem to distinguish between integer/double structures more thouroughly, but this might be overkill, but the amount of code has reduced by half! Path structures represent more like a class and work as a wrapper for std::vector. Also added some simplistic tests.

Depending on the feedback and available time, I could continue working on translating the rest of the codebase gradually. I find this as a great opportunity to learn more about C++, so don't expect high quality code either!

Naming conventions represent C++ style and code automatically formatted with clang-format.

Attaching the test projectwith implementation. Based on r555.

1 Attachments
ClipperTest.zip

Related

Bugs: #192

Discussion

  • Angus Johnson

    Angus Johnson - 2019-03-10

    Great, thanks!
    I'll have a look at it in the next day or two, and upload it to the sandbox. I've no problem with templates in C++. I've really only avoided them in Delphi to provide maximum compatability with older versions of Delphi. However, even with Delphi, I'm starting to wonder how much backward compatability is worthwhile asit does tend to make the code messier and hence harder maintain.

     

  • Angus Johnson

    Angus Johnson - 2019-03-12

    HI again Andrii.
    I've just had a look at your C++ translation and it look excellent, at least to my admittedly rudimentary knowledge of C++. However, there is one thing I'm tempted to change (or change back). I see your preference is to suffix "I" for integer related structures and functions (PointI, RectI etc). In the Delphi code I've been suffixing "64" to explicitly indicate storage size as I could see the possibility of also using 32bit integers (to optimize speed) and even 128 bit integers. Nevertheless, I'd agree that Point64, Rect64 isn't as pretty as PointI, RectI etc. Your thoughts??

     

  • Andrii Doroshenko

    Andrii Doroshenko - 2019-03-12

    Yeah, the reason I did so is more because I wanted to keep myself not distracted with these prefixes while translating, these can be easily changed. But at the same time, PathI has to be renamed to Path64(Path<int64_t>) as well becausePath<T> is a template here. Perhaps PathI chould be typedefed to default Path64. This is actually already done via typedefs:

    typedef Point<int64_t> PointI;
typedef Point<double> PointD;

    Another issue I noticed is that the compiler spits many warnings regarding implicit conversion from one type to another, I think these can be fixed somehow.

    Btw, yesterday I managed to translate Clipper.pas so it compiles at least, trying to make it actually work now (throwing access violation here and there).

     

    • Angus Johnson

      Angus Johnson - 2019-03-14

      Perhaps PathI chould be typedefed to default Path64.

      Yes, that might be the better solution. So if paths are defined as "PathI", it's a very easy to change a single typedef to redeclare it as a 32bit or 64bit (or whatever) integer. I guess it makes sense to revise this in the Delphi code too. I'm open to any other suggestions too :).

      Btw, yesterday I managed to translate Clipper.pas so it compiles at least, trying to make it actually work now (throwing access violation here and there).

      OK, thanks again. Once you've sorted out the AVs (or at least most of then :)) I'd be very happy to upload that to the Sandbox too.

      Again to clarify, I'm still some way from finishing Clipper2, and I've been putting off for some time the final hurdle, or at least having another break from one more less than satisfactory attempt at getting Clipper2 to where I really want it to be. Specifically, ClipperEx is still a work-in-progress. While it does mostly tidyup micro-intersections, it's not complete, nor does it address common-edges colinear edges of adjacent paths in clipping solutions. It's not that there aren't solutions, but I'm still searching for an elegant one , or at least one that isn't too convoluted with too many edge cases (pun intended) that's really messy and hard to maintain. And fixing ClipperEx will allow me to simplify the triangulation module too.

       

      • Angus Johnson

        Angus Johnson - 2019-03-14

        One significant reservation I do have over the I (eye) suffix I forgot to mention is how easily it can be confused with l (el) and with 1 (numeric). So, I guess I'm still open to persuasion on this one.

         

        Last edit: Angus Johnson 2019-03-14

        • Andrii Doroshenko

          Andrii Doroshenko - 2019-03-14

          True, I think having numeric suffix like 32/64/128 would be fine for integer stuff.

           

      • Andrii Doroshenko

        Andrii Doroshenko - 2019-03-14

        Yep, not touching ClipperEx.pas for now!

        Regarding AVs, those are difficult to fix without understading the inner workings, so I went for another approach and will try to incrementally translate from one revision to another... My translation skills are expanding nevertheless.

        I won't bother you much until I get it to work, or simply contact me if you're fine with half-done job. I'm highly dependent on your library and I'll try to speed things up as much as I can.

        Triangulation unit is particularly useful!

         

        • Angus Johnson

          Angus Johnson - 2019-03-14

          Regarding AVs, those are difficult to fix without understading the inner workings,

          Yes, that does make things very much harder. It's something I've encountered recently too trying to understand someone else's rather convoluted/complex C++ code. When code is unavoidably complex, it really needs to be well documented, and this is something I'm trying to address in my own coding (and at spending more time refactoring too). there. Anyhow, perhaps together we can get rid of the remaining AVs, especially if, as it seems, you've done most of the hard work :). You're welcome to email me about this.

           

          Last edit: Angus Johnson 2019-03-14

          • Andrii Doroshenko

            Andrii Doroshenko - 2019-03-15

            Managed to translate Clipper.pas at latest revision (r555) incrementally. Operations performed seem to work for simple paths (mainly rects), but I guess I messed up with something in the process of translation, see test_clipper.cpp in project attached with translation notes.

            Also replaced I suffix with Intand see how it works out.

            I didn't apply any formatting/style so that subsequent translations could be more easily done from Delphi one until stable version is released. Hopefully my work was helpful!

             
            ClipperCppR555.zip

            • Angus Johnson

              Angus Johnson - 2019-03-15

              Thanks! I'll look over it this weekend and hopefully upload it in the next few days.

              A couple of things so far ...

              1. I prefer the I suffix to Int which looks cumbersome. In monospace fonts (used by most code editors) I (eye) is usually clearly different from l (el) and 1 (one) so I'll revert to the I (eye) suffix. However, I'll need to watch this in the documentation (whenever I get around to attending to that).
              2. I'm not entirely happy with UnionRect, RotateRect, InflateRect & OffsetRect as static member functions. To my way of thinking they should be either independant functions, or renamed Rotate, Inflate etc, or (my preference) changed to non-static member functions and renamed Union, Inflate etc (just as Rotate is a member function of Point<t>). Then to be consistent across all classes, ScalePath(s), OffsetPath(s), etc should all become member functions of Path and Paths too. Your thoughts? </t>
               

              Last edit: Angus Johnson 2019-03-15

              • Andrii Doroshenko

                Andrii Doroshenko - 2019-03-16

                Hehe yes initally I did exactly that, renaming Rect methods and making them work from inside struct, but there was RotateRect that couldn't be overloaded (?) Anyway that's good idea.

                Same for making path(s) related functions to become member functions, but some technicalities could arrive from that as templates work the same for all types, and I've noticed some custom logic between scaling methods, for instance. They could be moved inside struct with little modifications though.

                Also operator=() could also be overloaded to convert between types to replace PathI ↔ PathD and similar conversions.

                I haven't touched ClipperD and related structures much as I didn't make the main unit work properly, but should be trivial to implement.

                 

                Last edit: Andrii Doroshenko 2019-03-16

                • Anonymous

                  Anonymous - 2019-03-17

                  OK, here's what I've done with clipper_core. Please bear in mind that I'm no C++ expert, so if there are things that aren't right or are unconventional I'm happy to be corrected. Also, I haven't looked at your clipper.h and clipper.cpp yet.

                   

                  Last edit: Angus Johnson 2019-03-17
                  ClipperTest.zip

                  • Andrii Doroshenko

                    Andrii Doroshenko - 2019-03-17

                    I'm sorry but it seems like you've uploaded the original version by mistake, I don't see any changes there, also make sure you downloaded ClipperCppR555.zip that I uploaded before which incorporates more changes to clipper_core.

                    clipper.h/clipper.cpp are a bit messy because I've given up trying to fix the bugs, so see for yourself, if you manage to fix it I'll be happy to clean it up if needed...

                     

                    • Angus Johnson

                      Angus Johnson - 2019-03-17

                      oops, try this ...

                       
                      clipper_core.zip

                      • Andrii Doroshenko

                        Andrii Doroshenko - 2019-03-17

                        Looks neat, apart from CamelCase vs snake_case naming convention which is a good idea to keep for now and will have to be converted as final touch in order to be more like C++'s STL.

                        I've noticed you've removed ClipperLibException from core. In fact I think it might be a good idea to define a macro so users can choose whether to have exceptions in code, the reason for that is that Clipper can be compiled for various platforms, and not all of them support exceptions (Android, iPhone etc)

                        While compiling I've got unresolved link to templated CrossProduct function at global scope, so should initialize these as well:

                        template double CrossProduct(const PointI &pt1, const PointI &pt2, const PointI &pt3);
template double CrossProduct(const PointD &pt1, const PointD &pt2, const PointD &pt3);

                        Also perhaps making a typedef for floating point type like cFloat would be an option? Or would this yield overflows if cFloat is float and not double?

                         

                        • Angus Johnson

                          Angus Johnson - 2019-03-17

                          Looks neat, apart from CamelCase vs snake_case naming convention

                          Yes, I mucked that up. My aim is to adhere to Google's C++ sytle (https://google.github.io/styleguide/cppguide.html) I see now I should be using snake_case for local variables.

                          I've noticed you've removed ClipperLibException from core.

                          Yes, my thinking there is it's probably better in clipper.h.

                          While compiling I've got unresolved link to templated CrossProduct function .

                          Yes, anopther error. Anyhow, I don't think that function needs to be templated.

                          Also perhaps making a typedef for floating point

                          I'm not sure that there's any benefit. To my understanding, it's unusual now for CPUs to be slower with double compared to float values.

                           

      • Andrii Doroshenko

        Andrii Doroshenko - 2019-03-14

        Not sure if you used translation utilities, but there's delphi2cpp that I'm using to partially translate Delphi code to C++, so this speeds up things tremendiously (with possible errors that arise from modyfing such code, haha)

         

  • Ignorant

    Ignorant - 2019-03-12

    good ..very good...
    Now how about a writeup in English about the methods/algorythms used by the
    new "oriented edges support" used by Angus.

    On Sun, Mar 10, 2019 at 5:56 PM Andrii Doroshenko xrayez@users.sourceforge.net wrote:

    Status: open
    Group: *
    Labels: Clipper2 C++
    Created: Sun Mar 10, 2019 12:26 PM UTC by Andrii Doroshenko
    Last Updated: Sun Mar 10, 2019 12:26 PM UTC
    Owner: nobody
    Attachments:

    Despite the fact that Clipper2 is still being worked on, I've translated
    some ClipperCore.pas structures/routines to C++. Current implementation
    uses templates as the library seem to distinguish between integer/double
    structures more thouroughly, but this might be overkill, but the amount of
    code has reduced by half! Path structures represent more like a class and
    work as a wrapper for std::vector. Also added some simplistic tests.

    Depending on the feedback and available time, I could continue working on
    translating the rest of the codebase gradually. I find this as a great
    opportunity to learn more about C++, so don't expect high quality code
    either!

    Naming conventions represent C++ style and code automatically formatted
    with clang-format.

    Attaching the test projectwith implementation. Based on r555.

    Sent from sourceforge.net because you indicated interest in
    https://sourceforge.net/p/polyclipping/bugs/192/

    To unsubscribe from further messages, please visit
    https://sourceforge.net/auth/subscriptions/

     

    Related

    Bugs: #192

    alternate

Anonymous
Anonymous

Add attachments
Cancel





MongoDB Logo MongoDB