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

[Oz Linden]

> 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
> 
>

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