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

Boroondas Gupte sllists at boroon.dasgupta.ch
Sun Mar 13 05:39:29 PDT 2011



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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110313/8f56b824/attachment.htm 


More information about the opensource-dev mailing list