> 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 > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
