Re: [brlcad-devel] Refactoring pipe element calculation in pipe.c

[Christopher S. M. <br...@ma...>]

On Apr 5, 2013, at 2:36 PM, Csaba Nagy wrote:

> It is likely a good idea to factor that calculation out in a separate
> function (rt_pipe_tess already has a slight variation of the algorithm,
> which is likely not a good sign).

Sounds like a great idea.  I'm a huge fan of the DRY principle.

> I uploaded a first attempt to it, which I discovered to be buggy (the
> curves have the outer diameter for the next point wrong).

You can keep uploading new patch files, just append a v2 or some other convention to distinguish later versions.

> After more code-reading, I see that there are already some structures to
> accommodate linear/bent elements in one list:
> 
> * id_pipe -> as a common header;
> * lin_pipe;
> * bend_pipe;
> 
> They are prepared by the rt_pipe_prep function and used in ray tracing I
> suppose (not checked that).

Yep, that's exactly what they're for.  rt_*_prep() precedes calls to rt_*_shot() during ray tracing for all objects.

> On further thinking that makes more and more sense - I just wanted to
> say that some of the pipe element processors don't need all what the
> lin/bend_pipe structures offer, and by reordering those structures so
> that the essential fields are in the beginning and the not so much used
> ones further down, I could define smaller structs to match them (same
> idea as id_pipe matches the beginning of lin_pipe/bend_pipe). Then a
> single function could init both kind of lists, fully calculated or
> simple - but now I think this is overkill and the fully resolved
> structure can always be calculated.

That does all make sense and sound reasonable to me too.  You could even just make the other functions call prep() directly so that it performs all the calculations (they're very trivial/fast), then callers would just read what they wanted.  Having a partially initialized/set structure is dangerous long-term.  Otherwise, the separate struct with those essential fields (and a func to init just those), which prep() would call would be fine too and not really any more complex.

Yet another viable approach... Pipe is one of the *very* few object types that was curiously implemented *without* defining a "specific" structure.  See the ell_specific struct in primitives/ell/ell.c for an example.  The general approach most objects take is that the rt_*_prep() function creates and fills in a specific structure (setting it in the st_specific field of the soltab stp arg).  The specific structure is set up to hold useful computations that you don't want to write to disk, but want to use (usually during ray tracing).  You could create one for pipe and put the essential items there.  You'd just have to be careful to preserve current behavior.

Cheers!
Sean