[opensource-dev] Review Request: Squared all dist_vec() based comparisons and other dist_vec() operations where sensible.

Oz Linden oz at lindenlab.com
Thu Mar 17 10:07:17 PDT 2011



> On March 12, 2011, 7:16 a.m., Oz Linden wrote:
> > indra/llcharacter/llbvhloader.cpp, line 1199
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1179#file1179line1199>
> >
> >     I think it would be clearer to either add a new constant POSITION_MOTION_THRESHOLD_SQUARED or to write these like
> >     
> >      ... < (POSTION_MOTION_THRESHOLD*POSITION_MOTION_THRESHOLD).
> >     
> >     (and same convention elsewhere)
> 
> Boroondas Gupte wrote:
>     For this specific line, you probably mean ">", not "<", do you?
>     
>     Also, what's the convention are you proposing? Grouping the multiplied values together by braces? Or leaving away the spaces around the multiplication operator?
> 
> Cron Stardust wrote:
>     Oz, I was thinking along those lines (creating or changing the constant,) but I wasn't sure if it would be within the scope of this.  After further thought, and evaluating the input thus far, I've come to the conclusion that it is within the scope.  A new diff incorporating the notes thus far mentioned will be forthcoming.

I meant putting the constant being squared inside parens with no spaces.  Really, if it's a constant I think it's probably better to make another constant with _SQUARED appended.

While I'm being nit-picky - I think it's clearer when wrapping an expression across lines to put the operator at the beginning of the second line instead of the end of the first line:

(dist_vec_squared(LLVector3(ki_prev->mPos), first_frame_pos) 
 > (POSITION_MOTION_THRESHOLD*POSITION_MOTION_THRESHOLD))


- Oz


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/199/#review448
-----------------------------------------------------------


On March 12, 2011, 11:54 p.m., Cron Stardust wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/199/
> -----------------------------------------------------------
> 
> (Updated March 12, 2011, 11:54 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> I looking at the code, trying to find out where/how to add a new feature, when I tripped across one of these and it lit my mental warning bells off.  Vector distance comparisons should, IMHO, always be done squared.  So I did some greppin, manual analysis, and some careful modification, and here's the result.
> 
> 
> This addresses bug VWR-25126.
>     http://jira.secondlife.com/browse/VWR-25126
> 
> 
> Diffs
> -----
> 
>   doc/contributions.txt 344d4c6d7d7e 
>   indra/llcharacter/llbvhloader.cpp 344d4c6d7d7e 
>   indra/llcommon/indra_constants.h 344d4c6d7d7e 
>   indra/llmath/tests/llbbox_test.cpp 344d4c6d7d7e 
>   indra/newview/llagent.cpp 344d4c6d7d7e 
>   indra/newview/llfloaterchat.cpp 344d4c6d7d7e 
>   indra/newview/llhudeffectlookat.cpp 344d4c6d7d7e 
>   indra/newview/llhudeffectpointat.cpp 344d4c6d7d7e 
>   indra/newview/llmaniprotate.cpp 344d4c6d7d7e 
>   indra/newview/llmanipscale.cpp 344d4c6d7d7e 
>   indra/newview/llnetmap.cpp 344d4c6d7d7e 
>   indra/newview/llpanelpeople.cpp 344d4c6d7d7e 
>   indra/newview/llselectmgr.cpp 344d4c6d7d7e 
>   indra/newview/llspeakers.cpp 344d4c6d7d7e 
>   indra/newview/llviewerchat.cpp 344d4c6d7d7e 
>   indra/newview/llviewermessage.cpp 344d4c6d7d7e 
>   indra/newview/llviewerparceloverlay.cpp 344d4c6d7d7e 
>   indra/newview/llvoicevivox.cpp 344d4c6d7d7e 
>   indra/newview/llworld.cpp 344d4c6d7d7e 
> 
> Diff: http://codereview.secondlife.com/r/199/diff
> 
> 
> Testing
> -------
> 
> Compiled a test viewer and used it, undertaking some of my normal activities.  Results felt good, but are currently anecdotal.  Any suggestions on how to properly measure this (or even some actual measurement from those already instrumented to measure these things,) would be great!
> 
> 
> Thanks,
> 
> Cron
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110317/1d3391fd/attachment-0001.htm 


More information about the opensource-dev mailing list