#280 [PATCH]Sketch primitive interface and implementation

Unstable
closed-accepted
None
5
2014-07-28
2014-06-09
Andrei Popescu
No

I've finished writing the interface for Sketch, but I've got some questions:

-Segment is represented as genptr_t in rtgeom.h so, it's really not an actual class, I didn't know how to design it, I simply made it an empty class with an internal pointer to genptr_t.
I don't know how to handle the arrays from C(int*, for example) should I use std vector container?

Any feedback is appreciated, thank you!

1 Attachments

Discussion

  • Andrei Popescu
    Andrei Popescu
    2014-06-11

    Finished sketch implementation, subelements and sketch itself.

    However, two questions.
    Is there any reason to have an adjust method for Sketch(since you have and can have get/setters)?
    How should it look like, if so?

    Similar question about order segments, cannot figure how it should look

    PS. Sorry for the delay.

    Andrei

     
  • Andrei Popescu
    Andrei Popescu
    2014-06-18

    Hello!

    • I've fully implemented Line, Arc and Nurb.
    • I haven't added the rest of Sketch functions yet, focused on implementing the rest.

    A few questions:

    How do we allocate the ctl_points array and respecitvely the k.knots array in Nurb?

    c_size says how "long" is the array, we need an internal counter to where we have gotten in the array. Since the nurb_seg doesn't contain it, I think it should be a field in Nurb class.
    As so, whenever the ctl_points array "fills" we double it's size, this is the most efficient way I could think of.

    PS: Patch compiles fine.

    Andrei

     
  • Sean Morrison
    Sean Morrison
    2014-06-18

    NURBS curve support is not fully implemented in sketch, but Bezier curves are. I suggest leaving NURBS curves off your list.

    That said, your double-the-size-when-empty reallocation strategy is quite common and usually suitable if you can't rely on a standard container. If you can rely on a standard C++ container (e.g., std::vector, std::map, std::array, etc), they're often preferred over manually managing memory but they are tricky with public headers (you have to use an Impl pattern to hide them from public API). Do whatever the other existing classes are doing. ;)

     
  • Andrei Popescu
    Andrei Popescu
    2014-06-19

    Sean,
    I've followed your advice, however:
    I didn't find any similar example(usage of a C like array) in other primitives.
    The bezier structure from rtgeom doesn't have a size parameter, so I wonder. How do we know when it's full?

    My solution was to add internal(in class) integers to track how many knots/points we put into the array.

    I've finished implementing all the subclasses, and I was looking at implementing main sketch functions, however, I don't understand this:

        /// selects a single object and hand it over to an SegmentCallback
        void                  Get(size_t                index,
                                  ConstSegmentCallback& callback) const;
    

    the ConstSegmentCallback doesn't have any constructor method that takes any parameter(object or not).

    The Segment manipulation(add, delete, append, insert etc) should be done relying internally on ->curve->segments?

    Thanks a lot!

    I attached a patch with all subclasses implemented.

     
    Last edit: Andrei Popescu 2014-06-19
  • Andrei,

    I've seen that you've still problems to handle the verts array. So I added some utility functions to make the work with this array easier.

    I did an example implementation for the line segment type. This example is untested/not compiled. It's left to you to test it and change the other segment types too.

    Furthermore: The Declaration of Type() in Segment is "= 0" which means that there is no implementation of this function in this class but in the child classes. And there Type() should test for valid pointers (see my Line example).

    Regarding ConstSegmentCallback: This class has to be implemented by the caller. You have to implement the Get() function. You may look at ConstDatabase::Get() to get an idea how.

    Regards,
    Daniel

     
    Attachments
  • Andrei Popescu
    Andrei Popescu
    2014-06-23

    Daniel,

    I've rewritten the .cpp and .h according to what we've discussed.

    So far, I've finished the subelements(Arc, Bezier, Nurb) and the Get methods from Sketch

    I wanted to break this in two, because the patch is already way too large.

    If this is ok, next I'll focus on Append/Insert methods and the U/V get/set, as well as Object inherited methods.

    However, I have a question:

    the genptr_t(void*) array is used for holding segments internal pointers. I haven't done this before, do I cast each element to a certain structure?

    Moreover, in the actual .h insert/append Line don't have any data(like start point, end point). Does that seem ok?

    Thanks a lot for your help so far,
    Andrei

     
  • Andrei,

    I wanted to break this in two, because the patch is already way too large.

    The whole patch (includes two files) is 1000 lines long. This isn't so much. If you like to split the implementation you could do it for the subclasses, i.e. SketchLine.cpp, SketchArc.cpp, etc.. But this may be not so easy as it sounds.

    the genptr_t(void*) array is used for holding segments internal pointers. I haven't done this before, do I cast each element to a certain structure?

    There is no genptr_t any more. Maybe you should update your sources. It's an void** now, i.e. an array of pointers pointing to something. To get the type of the something you have to test the magic. You already did this in the Get() function.

    Moreover, in the actual .h insert/append Line don't have any data(like start point, end point). Does that seem ok?

    Good Question. Adding the data would be easy for Line and Arc but not for the other two types. That's why I think the append and insert should consistently create an object with default values.

    Regards,
    Daniel

     
  • Andrei Popescu
    Andrei Popescu
    2014-06-26

    I have finished whole Sketch implementation, except isValid which I was unable to find or understand from sketch.c

    Any feedback is appreciated, thanks!

    Andrei

     
  • Andrei Popescu
    Andrei Popescu
    2014-06-28

    Daniel,
    I've fixed the issues you mentioned however I didn't know what to do with Sphere's modifications to Database and ConstDatabase since I didn't want to commit the Sphere code part and I couldn't commit locally either.

    I don't know if it works like this,but I'll keep looking on BoT.

    Thanks,
    Andrei

     
  • Andrei Popescu
    Andrei Popescu
    2014-06-29

    I've attached a clean sketch patch, will create a new bot one soon, as well

     
  • Andrei,

    I've reviewed at least a part of your patch. Here some notes how to go on:
    - I've added functions for the reverse flag to the header file. You need to implement them (not trivial).
    - The sketch has a lot of arrays. I changed the destructor accordingly. The copy constructor etc. still need some work.
    - Almost every operation on a segment has to take care that it isn't Null. I did it for the Line. You should change the others accordingly.
    - I changed the implementation of Append/InsertLine. You should change the others.
    - Look at the comments for carc_seg in rtgeom.h. Handle the mentioned special case (full circle). This is probable not trivial. (I renamed Arc to CircularArc but this shouldn't disturb you.)
    - Make it compile.

    I admit that Sketch is a very complex primitive.

    Daniel

     
    Last edit: Daniel Roßberg 2014-07-08
    Attachments
  • Andrei Popescu
    Andrei Popescu
    2014-07-14

    Daniel,
    I've finished the compiling issues(with a lot of help from you)
    I got the idea of what I should do, but I don t know how.

    1) the constructor issue
    I assume I need to use rt_curve_copy in the copy constructor, but it doesn't seem to work, pointer issues again. I left it commented.

    2) I've safeguarded all the segment(Arc, Nurb, Bezier ) operations like you did with Line. I don't understand why you put both asserts and if below, however.

    3) I've figured that if radius <= 0.0 (full circle) end is same as center, but I don't know/ don't see how that affects my AppendArc, InsertArc since no info is in them when they are added.

    4) it compiles.

     
    Last edit: Andrei Popescu 2014-07-14
    Attachments
  • 1)
    I attached a Sketch.cpp with changed copy constructor and = operator.

    2)
    Assert displays only a message but if changes the program flow.
    I asserted values which are probably a programming mistake. These values shouldn't appear in a well programmed software. Assert should display the programmer a warning about this fact. However, I don't allow to crash the interface with these wrong values by redirecting the program flow with if.

    3)
    It shouldn't affect the append/insert but the interface defined in class CircularArc.

    4)
    fine :)

    Daniel

     
    Attachments
  • Andrei Popescu
    Andrei Popescu
    2014-07-20

    I've figured out the issue with the Sketch Center, I hope it makes sense

    I've also corrected some mistakes, like EmbeddingPoint being Vector2D isntead of Vector3D. With Daniel's help, I've fixed all the issues mentioned in last feedback.

     
  • Sketch::DeleteSegment() has to remove the segments vertices from the verts and to free all memory allocated by the segment.
    Similar is true for the destructor Sketch::~Sketch().

    Daniel

     
  • Andrei Popescu
    Andrei Popescu
    2014-07-25

    Daniel,

    I fixed Sketch::DeleteSegment() by writing the deleteSegment()function

    I don't understand what was I supposed to do for Sketch::~Sketch() as it's already done.

    The destructor was actually written by you and, to me, it seems okay.

    Thanks,
    Andrei

     
  • Commited patch to the rt^3 branch. Further discussion will be on IRC and e-mail.

     
    • status: open --> closed-accepted
    • assigned_to: Andrei Popescu --> Daniel Roßberg
     
    • Group: Untested --> Unstable