> On March 13, 2011, 5:34 a.m., Boroondas Gupte wrote: > > indra/newview/llselectmgr.cpp, lines 6594-6595 > > <http://codereview.secondlife.com/r/199/diff/1-2/?file=1189#file1189line6594> > > > > While were at it, the original author probably meant "factor into" when > > they wrote "factor inside". And although the comment doesn't begin with a > > *complete* sentence, I'd start it with a capital letter when further > > sentences follow. > > So make this > > // Factor the distance into the displacement vector. > > This will get us > > // equally visible movements for both close and far > > away selections.
Good idea, but none of the other comments in the file start with a capital. Also, I'm not sure what the real meaning of the below is... > 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. > > Boroondas Gupte wrote: > Oh, and remove the trailing whitespace. Good call: I too have no idea what the meaning of "sqrt(min_dist) / 2" truely is, and I am a native speaker of English! I assumed that the sqrt was what was doing the factoring above, but I could be wrong; factoring to me means separating a number into its constituent factors, ie: 16 = 4 * 2 * 2. (Note that I'm not taking it into its prime factors, just the more general "factoring.") If my assumption is correct, then my variable name makes sense as it is the "min_dist" factored, otherwise I need some help grasping what was intended my this math. And thanks for the point-out on the trailing whitespace! I hadn't seen them, even though my editor has the whitespace set visible for just such a purpose! > On March 13, 2011, 5:34 a.m., Boroondas Gupte wrote: > > indra/llmath/tests/llbbox_test.cpp, line 37 > > <http://codereview.secondlife.com/r/199/diff/2/?file=1243#file1243line37> > > > > While we're changing this anyway, braces around the assigned define > > value probably won't hurt: > > #define APPROX_EQUAL(a, b) (dist_vec_squared((a),(b)) < 1e-10) > > > > They protect the define from unexpected operator precedence issues when > > used in certain contexts, so that it acts more similar to a function. > > > > Though, one has to wonder anyway, why a define is used here rather than > > a function. (Which could be declared inline, if really, really necessary.) The parens makes perfect sense: the lesser-than operator is pretty low on the precedence totem pole. I'll add them to the next revision, after some further discussion on some of the other topics. As to why it's not a function: I have no idea. C++ has way too many ways of doing the same task! :P > 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. > > Boroondas Gupte wrote: > [...] if this mainly occurs *at* comparisons [...] Good thoughts, however I think I'll have to defer to Oz on this one: I don't feel qualified to say one way or the other in this case. To me a helper function would clean up several lines of code in the codebase, but is it called for, and is it in the scope of this change? I cannot say... Oz? - Cron ----------------------------------------------------------- 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
