Menu

#300 [PATCH]Pipe primitive interface and implementation

Incomplete
closed-accepted
None
5
2015-08-10
2014-08-04
Anonymous
No

Daniel,

It took me a bit more because, honestly, I haven't been working as hard, but I finished the pipe primitive interface and started writing the .cpp.

If you can, please review this as soon as possible so I can adjust my .cpp.

Thanks,
Andrei

1 Attachments

Discussion

  • Daniel Roßberg

    Daniel Roßberg - 2014-08-06

    Andrei,

    I attached an alternative version of the header file. The main difference is that I named the segment "ControlPoint". This name makes it easier to understand. The pipe is a list of connected points with additional radius attributes in these points.

    My version of the interface isn't perfect too, but the work on it wan't be for nothing. With your help I've now some primitives with complex substructures which I've to harmonize.

    Regards,
    Daniel

     
  • Andrei Popescu

    Andrei Popescu - 2014-08-14

    Finished pipe implementation, followed bot as example and pipe.c for understanding how Pipe works

     
  • Daniel Roßberg

    Daniel Roßberg - 2014-08-19

    Destructor: Using bu_list_free() is possible, maybe an additional test for a valid list is neccessara (not sure), but m_internalp has to be freed too.

    AppendControlPoint()/InsertControlPoint(): ctlPoint has no memory assigned.

    DeleteControlPoint(): The dequeued structure isn't freed.

    Daniel

     
  • Ilinca Andrei

    Ilinca Andrei - 2015-03-29

    Hello, I ran into this ticket while I was documenting about the C++ Geometry API and I thought it would be a good idea to fix the remaining tasks.

    Initially, I had some errors because of CMAKE (I used a workaround when installed brlcad from svn sources) and I had to first fix those to make it compile.

    Then, I applied the patch and figured there was a CMAKE conflict, which I fixed by removing the CMAKE part from the patch.

    SVN doesn't have commit locally so I needed an workarond to showcase my changes.

    Files:
    fixed_pipe_implementation.patch contains the whole, functional patch that applies smoothly with the feedback applied

    changes_made.diff is what I made

     

    Last edit: Ilinca Andrei 2015-03-29
  • Daniel Roßberg

    Daniel Roßberg - 2015-03-30

    Hm, allocating memory with new and freeing with bu_free() won't work. You should test your code before committing next time ;)

    Daniel

     
  • Ilinca Andrei

    Ilinca Andrei - 2015-03-30

    Thanks for the feedback, Daniel!

    Sorry, I didn't look in other files for an example.

    Here are the the rewritten patches, according to your review.

    I rebuilt rt^3 and ran ./tester_ci_primitives on a random .g file (found in brlcad/src/librt/tests).

    Regards,
    Andrei

     

    Last edit: Ilinca Andrei 2015-03-30
  • Daniel Roßberg

    Daniel Roßberg - 2015-04-09

    Hi Kalpit,

    Two issues:
    1) I get the following error when I try to compile the tests:
    [ 82%] Building CXX object tests/coreInterface/CMakeFiles/tester_ci_primitives.dir/ellipsoid.o
    In file included from /home/rossberg/Devel/BRL-CAD/rt^3/tests/coreInterface/ellipsoid.cpp:34:0:
    /home/rossberg/bin/brlcad/include/brlcad/raytrace.h:41:17: fatal error: tcl.h: File or directory not found
    #include "tcl.h"
    ^
    compilation terminated.
    tests/coreInterface/CMakeFiles/tester_ci_primitives.dir/build.make:77: recipe for target 'tests/coreInterface/CMakeFiles/tester_ci_primitives.dir/ellipsoid.o' failed
    make[3]: [tests/coreInterface/CMakeFiles/tester_ci_primitives.dir/ellipsoid.o] Error 1
    CMakeFiles/Makefile2:322: recipe for target 'tests/coreInterface/CMakeFiles/tester_ci_primitives.dir/all' failed
    make[2]:
    [tests/coreInterface/CMakeFiles/tester_ci_primitives.dir/all] Error 2
    CMakeFiles/Makefile2:334: recipe for target 'tests/coreInterface/CMakeFiles/tester_ci_primitives.dir/rule' failed
    make[1]: [tests/coreInterface/CMakeFiles/tester_ci_primitives.dir/rule] Error 2
    Makefile:226: recipe for target 'tester_ci_primitives' failed
    make:
    [tester_ci_primitives] Error 2

    Can you reproduce this?

    2) Which code formatting conventions do you follow?

    Daniel

     
  • Kalpit Thakkar

    Kalpit Thakkar - 2015-04-10

    Hi Daniel.
    Answers :

    1) Yes, I can reproduce the error (Here : https://gist.github.com/dracarys-stormborn/b00acf3f9350d5f91f86). And the reason for why that happens is running cmake in build directory more than once.
    Well, I ran into this error before as well, but my workaround was to remove the build directory altogether and run cmake once and then build the project. Now that you have run into this error as well, I tried to look into the problem deeper and I examined "flags.make" in the corresponding target directories (ge.dir, tester_ci_primitives.dir, so on).
    What I found is : First time you run cmake, the "CXX_FLAGS" are written correctly. Running cmake any number of times henceforth, the path "<BRLCAD-INSTALL-DIR>/dev-7.25.0/include" goes missing (In my case, it is "/usr/brlcad/dev-7.25.0/include"). It puts "<BRLCAD-INSTALL-DIR>/dev-7.25.0/include/brlcad" as one of the include paths in "CXX_FLAGS", but you may see that it doesn't have tcl.h there - nor do the other include paths have it and hence the error.
    I have not found the solution to this yet, sadly. I'm trying but I will need your help with doing that, as I have little knowledge about CMake.
    For now, to review my patch, you can remove the present rt^3 build directory and build it again, running cmake only once. It doesn't take much time so it's not painful to do that, I guess. :)

    2) I use sublime text 3 as my text editor right now. The formatting conventions, that I follow are according to the footer in the file generated by sh/template.sh. I used :
    "tab-size" : 4
    "translate_tabs_to_spaces": false,
    "use_tab_stops": true

    Regards,
    Kalpit

     
  • Daniel Roßberg

    Daniel Roßberg - 2015-04-13

    Hi Kalpit,

    1) I made the necessary changes to the CMake configuration.

    2) Your formatting is inconsistent. Your shiftwidths vary from 2 to 8. Another example: tests/coreInterface/primitives.cpp has the right footer but you are using only tabs for indents. At least the tests should follow the rules from HACKING.

    Daniel

     
  • Daniel Roßberg

    Daniel Roßberg - 2015-04-18

    Kalpit,

    Do you know, that the BRL-CAD code conventions require 4 spaces for the first, a tab for the second, a tab and 4 spaces for the third level, etc. indent?

    Your patch should be so that it can be committed without any change to the repository.

    Daniel

     
  • Daniel Roßberg

    Daniel Roßberg - 2015-07-27

    Kalpit,
    please correct (if possible) the license statement in tests/coreInterface/pipe.cpp and tests/coreInterface/sphere.cpp.

    Daniel

     
  • Kalpit Thakkar

    Kalpit Thakkar - 2015-07-29

    Daniel,
    I have corrected the license statement in the files you mentioned. The two updated patches are attached.

    Kalpit

     
  • Daniel Roßberg

    Daniel Roßberg - 2015-08-04
    • assigned_to: Andrei Popescu --> Daniel Roßberg
     
  • Daniel Roßberg

    Daniel Roßberg - 2015-08-05

    Hmm, there is an issue you could (or better should) solve:
    In Pipe::~Pipe() bu_free() isn't enough. You have to free all entries of the list m_internalp->pipe_segs_head as well.

    Please provide the code for the Pipe::~Pipe() method only (not the whole patch).

    Daniel

     
  • Kalpit Thakkar

    Kalpit Thakkar - 2015-08-06

    Daniel,
    The function Pipe::~Pipe() is attached as a text file.

    Regards,
    Kalpit

     
  • Daniel Roßberg

    Daniel Roßberg - 2015-08-10
    • status: open --> closed-accepted
     
  • Daniel Roßberg

    Daniel Roßberg - 2015-08-10

    Accepted with svn revision 65863.

     

Log in to post a comment.