> 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 [...] > > Cron Stardust wrote: > 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? > > Boroondas Gupte wrote: > Should we tag the jira issue with the label "needs-design" or is that for > user-visible design only?
needs-design is just for UI While conceptually I like the idea of the helper function, the main motivation for this is performance, so I'd say leave it as is without the helper function. > 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.) > > Cron Stardust wrote: > 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 > > > > Boroondas Gupte wrote: > It's almost like perl, isn't it? ;-) It's not as good as perl :-) > 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. > > Cron Stardust wrote: > 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! > > Boroondas Gupte wrote: > I'm pretty sure no "factoring" in the sense of "split into factors" takes > place here. As written above, I think that when writing "factor [A] inside > [B]", the comment author actually meant "factor [A] into [B]" (in the meaning > of "work [A] into [B]", i.e., take [A] into account, make it part of the > result [B], have [A] influence [B]). > > The function at hand gets called when objects are being moved with a > SpaceNavigator or joystick. I'm not completely sure, but I guess the math > here is to take perspective foreshortening into account, so that you get the > same apparent (i.e. onscreen) movement for far and nearby selections for the > same joystick input. (I.e., in 3D space, the far selections will actually > move faster than nearby ones would.) I'm not really sure why the nearestby > object of the selection determines the selection's distance, rather than > -say- the center of mass. > > Though, isn't perspective foreshortening inverse linear to the distance, > due to the http://en.wikipedia.org/wiki/Intercept_theorem ? Did someone > confuse lines and areas here? Or would actual scaling with the distance be > too extreme, while no scaling would feel strange too, so the square root is > some kind of compromise? > > The more I think about it, the more obscure this calculation seems to me. Let's not try to fix the naming here with respect to whatever 'factor' was supposed to mean. - Oz ----------------------------------------------------------- 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
