[PATCH]Pipe primitive interface and implementation
Open Source Solid Modeling CAD
Brought to you by:
brlcad
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
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
Finished pipe implementation, followed bot as example and pipe.c for understanding how Pipe works
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
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
Hm, allocating memory with new and freeing with bu_free() won't work. You should test your code before committing next time ;)
Daniel
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
Hello Daniel.
Sorry for taking so long. Well, I have tested the pipe implementation with the patch submitted by Ilinca. I faced some problems while testing and it took some time to understand where it was coming from.
I found the problems; tried my best to fix them the right way. I have attached the final patch. The other testing files that I wrote before which would be needed to apply this patch successfully can be found here :
Ellipsoid : https://gist.github.com/dracarys-stormborn/ef9aba47315f3bf683ba
Sphere : https://gist.github.com/dracarys-stormborn/e3c470bca0a7c233070a
Cone : https://gist.github.com/dracarys-stormborn/d0e14d53e219084f9831
Hoping to see more challenges :D
Regards,
Kalpit
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
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
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
Hi Daniel,
I have made the necessary changes and also made the changes in the formatting conventions. I had not indented the whole file consistently before, indenting only where I made changes. I hope that now my files are consistent with the rules from HACKING.
The updated patches are attached.
Regards,
Kalpit
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
Hello Daniel!
It took me some time to test the patches as I had to migrate to an older version to make rt^3 work. I have broken down the changes into small patches so that it becomes easy to understand. All the patches have to be applied for it to work.
The indentation rules I followed are according to what you mentioned before. I hope this one works well. I will work on making rt^3 in accordance with the present header changes in BRL-CAD soon.
Regards,
Kalpit
PS : I had checked out r64556 (randomly chosen).
Kalpit,
please correct (if possible) the license statement in tests/coreInterface/pipe.cpp and tests/coreInterface/sphere.cpp.
Daniel
Daniel,
I have corrected the license statement in the files you mentioned. The two updated patches are attached.
Kalpit
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
Daniel,
The function Pipe::~Pipe() is attached as a text file.
Regards,
Kalpit
Accepted with svn revision 65863.