Mainly just focused on eliminating the allocs while raytracing. Moved most of the dynamic stuff to the prep function.
svn diff output
Did a quick review and there are a few problems with the patch, some that need fixing.
Looks like the patch has some debugging statements left in it that shouldn't be applied. (brep.cpp, ebm.cpp, maybe more)
There is mixed style in the edits. Per our dev guide (HACKING file), the code should be using k&r/knf/stroustrup style instead of bsd/allman/java style for consistency sake.
What's the implication of the MAXDEPTH 30 constant? Didn't get through all of the logic to infer what that impacts -- does that mean only 30 segments on a given shotline through an extrusion?
There are a few stray edits that shouldn't be in there -- whitespace changes to a few random files. (src/rt/main.c, src/librt/shoot.c) Those are pretty innocuous and would normally be ignored, but given there are other issues, an update could leave them out. Often easiest to just run svn diff on the specific files you want included.
fixed the issues addressed above.
Ok I have attached a new patch with the issues outlined below addressed. We discussed the MAXDEPTH today but for documentation purposes that settings is controlling how many times to subdivide the bezier in order to get down to a single segment (this would support up to 2^64 control points). Note though this was a const that was there before in the bezier evaluator. I only modified 3 files nurbs.h, bezier_2d_isect.c and extude.c. I have also modified my editor settings to be compliant with the HACKING standards. Please let me know if there is anything else so I can correct in my settings.
Hey David, I checked out the patch, applied it, and ran it through some tests and it doesn't seem to be working. There were a couple other problems with the patch too. As a general rule, you shouldn't include style/formatting changes in a patch along with logic changes as they are relatively indistinguishable from each other. The whitespace changes just make it nearly impossible to find the actual changes and make it much more likely to cause conflicts when the patch is applied (which yours did conflict due to other ws changes). If the style was an automatic thing performed by the editor, perhaps it can be turned off for the purpose of isolating the patch. If you manually inspect a patch file, it should only have +/- lines for the actual lines edited. That's pretty universal across contributing to any open source project.
As for the style that was applied, there were a few inconsistencies that still could be tweaked but I'll hold off going into those for the time being. The more important issue was that the patch didn't seem to work. My quick test cases all crashed with various stack traces and memory allocation/deallocation errors so I didn't go any further.
Is there any update to this patch? The optimization would be great to have, but some changes were required to the patch that haven't been addressed. Please let us know if there's no intention of updating the patch so the tracker item can be closed out.
This Tracker item was closed automatically by the system. It was
previously set to a Pending status, and the original submitter
did not respond within 14 days (the time period specified by
the administrator of this Tracker).
Log in to post a comment.