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

[Boroondas Gupte]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/199/#review453
-----------------------------------------------------------


Thanks for the changes. Some more comments from me:


indra/newview/llselectmgr.cpp
<http://codereview.secondlife.com/r/199/#comment329>

    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.



indra/newview/llselectmgr.cpp
<http://codereview.secondlife.com/r/199/#comment330>

    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.



indra/llmath/tests/llbbox_test.cpp
<http://codereview.secondlife.com/r/199/#comment331>

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



indra/newview/llfloaterchat.cpp
<http://codereview.secondlife.com/r/199/#comment332>

    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


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/20110313/a65f62f0/attachment-0001.htm