> On March 13, 2011, 5:34 a.m., Boroondas Gupte wrote: > > indra/newview/llselectmgr.cpp, lines 6596-6599 > > <http://codereview.secondlife.com/r/199/diff/1-2/?file=1189#file1189line6596> > > > > Notwithstanding the comment above this code, I don't think the notion > > of a "factored minimal distance" makes any sense at all. (I might be > > mistaken, as I'm not a native speaker of English.) > > > > If we can't think of a better name (e.g. based on the actual geometric > > meaning of this intermediary result), just describe what we're storing > > here, e.g. with an ugly name like half_sqrt_of_min_dist. > > > > Because the variable is only used very locally and the computation of > > its value isn't too complicated, just naming it "tmp" or similar might also > > be acceptable, and still better than a name purporting a wrong or confusing > > meaning.
Oh, and remove the trailing whitespace. > On March 13, 2011, 5:34 a.m., Boroondas Gupte wrote: > > indra/newview/llfloaterchat.cpp, lines 416-419 > > <http://codereview.secondlife.com/r/199/diff/2/?file=1245#file1245line416> > > > > If storing return values of functions (or inline computations), just so > > we don't have to call the function (or perform the calculation) twice to > > get its square is a reoccurring pattern, we might want a helper function > > that'll save us from having to introduce extra variables just for this > > purpose: > > > > // Either > > F32 sqr(F32 base) { return (base * base); } // Isn't something like > > this already available in the standard library or llmath? Maybe even as a > > template? > > > > // and then here > > F32 distance_squared = > > dist_vec_squared(pos_agent, chat.mPosAgent); > > if (distance_squared > > > sqr(gAgent.getNearChatRadius())) > > // ... > > > > // or, if this mainly occurs comparisons between distances to other > > values > > bool dist_vec_leq( LLVector3 first_position, LLVector3 second_position, > > F32 distance) > > { > > return ( dist_vec_squared(first_position, second_position) <= > > (distance * distance) ); > > } > > > > // and then here > > > > if (!dist_vec_leq(pos_agent, chat.mPosAgent, > > gAgent.getNearChatRadius())) > > // ... > > > > Off course, where intermediary variables help readability or > > understandability, we should prefer them, but I don't think that's the case > > here. [...] if this mainly occurs *at* comparisons [...] - Boroondas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/199/#review453 ----------------------------------------------------------- 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
