[opensource-dev] Review Request: Fixed snapping of rotation in the edge-on case

[Cron Stardust]

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

Review request for Viewer.


Description
---

Simply had to guard the snapping code, making sure that the last "else" case 
was preserved when either the outer or the inner tests failed.

Careful analysis and testing was performed to determine what clauses 
could/should be inside the guarding if statement, which led to the discovery 
that the else clause had to be preserved outside under a separate test.  Code 
structure was carefully designed to match the other llManip* classes in similar 
areas.


This addresses bug STORM-1919.
https://jira.secondlife.com/browse/STORM-1919


Diffs
-

  doc/contributions.txt 9505109727a3e948b11564910fd58ee93503b3df 
  indra/newview/llmaniprotate.cpp 9505109727a3e948b11564910fd58ee93503b3df 

Diff: http://codereview.secondlife.com/r/608/diff/


Testing
---

Tested all rotation cases (edge-on & facing multiplied with snapping enabled & 
disabled) in Firestorm - however this area of code has not been modified in 
either viewer since ancient times and should therefore cause no ill effects.


Thanks,

Cron Stardust

___
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

Re: [opensource-dev] Review Request: Allow scripts to be saved/loaded to/from files.

[Cron Stardust]

> On Nov. 20, 2011, 6:53 a.m., Lance Corrimal wrote:
> > found another glitch:
> > I'm developing scripts in eclipse with lslforge. When I "compile" a script 
> > in eclipse and then load it using this patch, tabstops are converted into 
> > IDunnoWhat (it shows as little squares in the script editor).
> >
> 
> Ima Mechanique wrote:
> I'm not familiar with lslforge, but I'll look into it. Are you sure 
> they're valid tabs? I ask because it handles tabs from LSL Editor without a 
> problem.
> 
> Ima Mechanique wrote:
> Looks like this is a bug I've introduced recently, as it is also 
> affecting my LSL Editor created files now. :-(
> Trying to track it down, as soon as I can work out how to go back to a 
> specific revision in Hg.
> 
> Ima Mechanique wrote:
> Damn.  SL's text editing windows do not like tabs. When copy/pasting the 
> tabs are converted to spaces, but not when inserting text. I'll look at 
> finding a way around this.
> 
> Kadah Coba wrote:
> I don't remember any in-viewer text input ever liking \t particulary. It 
> might be best, or at least easier or safer, to just convert them to spaces on 
> import.
> 
> Cron Stardust wrote:
> I use an external editor now for all my scripting (TextWrangler) and I 
> have set it to use only tabs for all indentation.  The viewer accepts it just 
> fine, and will even regurgitate the tabs when I open the same script in the 
> external editor sometime later.  However, if I try to edit the text using the 
> in-world editor it will mangle the tabs every time.  So my suggestion is 
> this: don't worry about tabs when saving or loading scripts - leave the text 
> as-is.  If it's too annoying (which I think it is) than a JIRA entry should 
> be made, unless there is one already, to request that the SL text editor 
> support real tabs (\t) in the text.  That's where the real problem domain is, 
> not here, IMHO.
> 
> Ima Mechanique wrote:
> @Kadah, @Cron. Guys, tabs are now handled as noted it the last diff 
> (below). Are you suggesting I should remove the conversion?
> Personally I like using tabs in my external editors, but hate the boxes 
> in SL's editor.
> 
> @Cron. What editor is that? I've only tried live editing externally with 
> LSL Editor, set to use UTF-8 and only indent with tabs, but the tabs only 
> show as boxes in SL which I find defeats the purpose of clear indentation 
> (and ugly too).
> 
> Kadah Coba wrote:
> I had the mistaken impression from my experience that part of that issue 
> was at least the fault of more than just the UI text editor controls. I was 
> only making the point cause the tab issue and the fact that most external 
> editors use \t for tabs, is likely going to lead to "I uploaded a script and 
> now I can't edit it properly in-viewer, thus the uploader is broken". 
> I'd like \t to be preserved on the uploads and simple have a jira 
> regarding the text editor's poor handling of \t linked to STORM-1708, then 
> get that fixed separately.

@Ima: WRT to the conversion, I suspect controversy here.  Personally I'd go for 
leaving the file the way I intended it - with tabs, or any number of spaces, 
depending on how my external editor is configured.  To me, since I use 
ExternalEditor, this Save/Load functionality is more a way of doing backup and 
restore, but having it mangle my files doesn't sound good to me.  Maybe a debug 
setting for the conversion, but most likely not: the real fix is to fix the 
in-world editor to display the tabs correctly.  Like I said in the OSDev list, 
I don't see the squares: the tabs render as something akin to spaces.

As to the editor, it's just a nice advanced editor for Mac OS that's free.  Its 
big brother is BBEdit.  I had to go find a syntax highlighter module for LSL.  
In Windows I use Programmer's Notepad, again with a highlighter plugin.  On 
Linux, since I use KDE, I set up Kate as my editor.  All three are very similar.

@Kadah: Agreed.


- Cron


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


On Nov. 22, 2011, 5:17 p.m., Ima Mechanique wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/516/
> ---
> 
> (Updated Nov. 22, 2011, 5:17 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Chan

Re: [opensource-dev] Review Request: Allow scripts to be saved/loaded to/from files.

[Cron Stardust]

> On Nov. 20, 2011, 6:53 a.m., Lance Corrimal wrote:
> > found another glitch:
> > I'm developing scripts in eclipse with lslforge. When I "compile" a script 
> > in eclipse and then load it using this patch, tabstops are converted into 
> > IDunnoWhat (it shows as little squares in the script editor).
> >
> 
> Ima Mechanique wrote:
> I'm not familiar with lslforge, but I'll look into it. Are you sure 
> they're valid tabs? I ask because it handles tabs from LSL Editor without a 
> problem.
> 
> Ima Mechanique wrote:
> Looks like this is a bug I've introduced recently, as it is also 
> affecting my LSL Editor created files now. :-(
> Trying to track it down, as soon as I can work out how to go back to a 
> specific revision in Hg.
> 
> Ima Mechanique wrote:
> Damn.  SL's text editing windows do not like tabs. When copy/pasting the 
> tabs are converted to spaces, but not when inserting text. I'll look at 
> finding a way around this.
> 
> Kadah Coba wrote:
> I don't remember any in-viewer text input ever liking \t particulary. It 
> might be best, or at least easier or safer, to just convert them to spaces on 
> import.

I use an external editor now for all my scripting (TextWrangler) and I have set 
it to use only tabs for all indentation.  The viewer accepts it just fine, and 
will even regurgitate the tabs when I open the same script in the external 
editor sometime later.  However, if I try to edit the text using the in-world 
editor it will mangle the tabs every time.  So my suggestion is this: don't 
worry about tabs when saving or loading scripts - leave the text as-is.  If 
it's too annoying (which I think it is) than a JIRA entry should be made, 
unless there is one already, to request that the SL text editor support real 
tabs (\t) in the text.  That's where the real problem domain is, not here, IMHO.


- Cron


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


On Nov. 22, 2011, 5:17 p.m., Ima Mechanique wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/516/
> ---
> 
> (Updated Nov. 22, 2011, 5:17 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Changes to allow opened script window to save/load to/from files on the users 
> computer.
> 
> 
> This addresses bug https://jira.secondlife.com/browse/storm-1708.
> 
> http://jira.secondlife.com/browse/https://jira.secondlife.com/browse/storm-1708
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt a1319d553db9 
>   indra/llui/lltexteditor.h a1319d553db9 
>   indra/llui/lltexteditor.cpp a1319d553db9 
>   indra/newview/llfilepicker.h a1319d553db9 
>   indra/newview/llfilepicker.cpp a1319d553db9 
>   indra/newview/llfloaternamedesc.h a1319d553db9 
>   indra/newview/llfloaternamedesc.cpp a1319d553db9 
>   indra/newview/llpreviewscript.h a1319d553db9 
>   indra/newview/llpreviewscript.cpp a1319d553db9 
>   indra/newview/llviewerfloaterreg.cpp a1319d553db9 
>   indra/newview/skins/default/xui/en/panel_script_ed.xml a1319d553db9 
> 
> Diff: http://codereview.secondlife.com/r/516/diff
> 
> 
> Testing
> ---
> 
> Successfully opened and saved scripts from/to local files.
> 
> 
> Thanks,
> 
> Ima
> 
>

___
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

Re: [opensource-dev] Review Request: storm-1663: widen pitch clamping on camera, make it the same for sitting

[Cron Stardust]

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

Ship it!


Looks good to me.  The only issue I can see is commenting to why it's limited 
to 1 degree off the pure limit, so as to prevent this kind of confusion again.  
My guess is to prevent gimbal lock - which could then be rectified a different 
way later in the future, for instance by eliminating the limits and using 
non-Euler angles to control the rotation allowing the user to rotate 
continuously in vertical direction like is done in the horizontal.

- Cron


On Oct. 23, 2011, 6:44 a.m., Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/504/
> ---
> 
> (Updated Oct. 23, 2011, 6:44 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> See linked jira issue.   It's not yet clear why the existing limits were 
> chosen, or why the limits were narrowed when sitting.
> 
> 
> This addresses bug storm-1663.
> http://jira.secondlife.com/browse/storm-1663
> 
> 
> Diffs
> -
> 
>   indra/newview/llagent.cpp 02cd1e33128c 
> 
> Diff: http://codereview.secondlife.com/r/504/diff
> 
> 
> Testing
> ---
> 
> The changes here are essentially the same as the method used in Phoenix, so I 
> don't expect anything catastrophic.
> 
> test viewer at 
> http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/oz_project-2/rev/243714/index.html
> 
> 
> Thanks,
> 
> Oz
> 
>

___
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

Re: [opensource-dev] Review Request: STORM-1459 "Wearing Tab" - Add ability to copy displayed inventory names to clipboard

[Cron Stardust]

> On June 30, 2011, 11:02 a.m., Vadim ProductEngine wrote:
> > indra/newview/llpanelwearing.cpp, lines 297-302
> > 
> >
> > Looks a bit confusing.
> 
> Lance Corrimal wrote:
> but it's elegant!

Just needs some parens:
text += (iter != data.end()) ? item->getName() + "\n" : item->getName();

But otherwise it looks just like a a lot of lines I've run across in the past, 
and with the comment it's not all that difficult to understand.


- Cron


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


On June 30, 2011, 1:27 a.m., Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/370/
> ---
> 
> (Updated June 30, 2011, 1:27 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Add a feature on the "Wearing TAB" where users could copy to the clipboard 
> everything you see in the "Wearing TAB". This would make the blogging 
> communities life soo much easier instead of having to type out all that 
> information.
> 
> The label on this button needs input from someone on the XD team.
> 
> I would like to know if my code for adding a CR to the end of every line but 
> the last one could be done in a more elegant way.
> 
> 
> This addresses bug STORM-1459.
> http://jira.secondlife.com/browse/STORM-1459
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt f9864a43ddf0 
>   indra/newview/llpanelwearing.h f9864a43ddf0 
>   indra/newview/llpanelwearing.cpp f9864a43ddf0 
>   indra/newview/skins/default/xui/en/menu_wearing_gear.xml f9864a43ddf0 
> 
> Diff: http://codereview.secondlife.com/r/370/diff
> 
> 
> Testing
> ---
> 
> Clicked on Send to Clipboard button and was able to paste results into an 
> editor.
> 
> 
> Thanks,
> 
> Jonathan
> 
>

___
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

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

[Cron Stardust]

> On April 4, 2011, 4:39 p.m., Cron Stardust wrote:
> >

Huh.. long winded speech became dust on pressing publish...  Here goes again:


- Cron


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


On April 4, 2011, 10:34 a.m., Cron Stardust wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/199/
> ---
> 
> (Updated April 4, 2011, 10:34 a.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

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

[Cron Stardust]

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


- Cron


On April 4, 2011, 10:34 a.m., Cron Stardust wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/199/
> ---
> 
> (Updated April 4, 2011, 10:34 a.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

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

[Cron Stardust]

> On April 4, 2011, 2:33 p.m., Boroondas Gupte wrote:
> > indra/newview/llselectmgr.cpp, lines 6591-6592
> > <http://codereview.secondlife.com/r/199/diff/1-2/?file=1189#file1189line6591>
> >
> > This comment only really makes sense to those aware of this change. 
> > Those reading the code later won't easily be able to make any sense of it.
> > 
> > As a rule of thumb, in-code comments should relate to the resulting 
> > code and commit messages to the change. If you feel you have to deviate 
> > from that, make it explicit. E.g. here, we could write:
> > 
> > // Replaces a call to dist_vec(), which uses fsqrtf. 
> > Thus that's what we use here, too.
> > F32 min_dist = fsqrtf(min_dist_squared);

The comment is a warning to those who might notice that a few lines further 
down a different sqrt function is used.  Your edition is clearer though.


> On April 4, 2011, 2:33 p.m., Boroondas Gupte wrote:
> > indra/newview/llselectmgr.cpp, lines 6595-6597
> > <http://codereview.secondlife.com/r/199/diff/2-3/?file=1252#file1252line6595>
> >
> > While we're touching this code anyway, put spaces around (i.e. on both 
> > sides of) the binary * operators (multiplication). That makes it easier to 
> > visually distinguish them from unary * operators (dereference).

How did I miss doing that? :P


> On April 4, 2011, 2:33 p.m., Boroondas Gupte wrote:
> > indra/newview/llselectmgr.cpp, lines 6594-6597
> > <http://codereview.secondlife.com/r/199/diff/2-3/?file=1252#file1252line6594>
> >
> > Even if we decided that it is out of scope to decide what "factor" 
> > might have meant here, we can avoid having to place a FIXME comment here by 
> > just using a variable name that avoids the term.
> > 
> > half_sqrt_of_min_dist is admittedly an ugly name and doesn't tell the 
> > reader why we would calculate it, but it is at least truthful and not 
> > misleading, so I'd really go for that for now.

ugh.. still bugs me.  I agree that the FIXME is obnoxious, and your variable 
name (as ugly as I agree it is,) is only 4 characters longer.  Lemme try an 
idea in a parallel review of my own, let me know what you think.


- Cron


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


On April 4, 2011, 10:34 a.m., Cron Stardust wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/199/
> ---
> 
> (Updated April 4, 2011, 10:34 a.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

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

[Cron Stardust]

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

(Updated April 4, 2011, 10:34 a.m.)


Review request for Viewer.


Changes
---

Following the review comments above, I parenthesized a #define to make it 
safer, adjusted some notes (and added a TODO) around some extremely obscure 
code that needs further attention but which is outside this scope.


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 (updated)
-

  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

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

[Cron Stardust]

> On March 12, 2011, 7:16 a.m., Oz Linden wrote:
> > indra/llcharacter/llbvhloader.cpp, line 1199
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1179#file1179line1199>
> >
> > I think it would be clearer to either add a new constant 
> > POSITION_MOTION_THRESHOLD_SQUARED or to write these like
> > 
> >  ... < (POSTION_MOTION_THRESHOLD*POSITION_MOTION_THRESHOLD).
> > 
> > (and same convention elsewhere)
> 
> Boroondas Gupte wrote:
> For this specific line, you probably mean ">", not "<", do you?
> 
> Also, what's the convention are you proposing? Grouping the multiplied 
> values together by braces? Or leaving away the spaces around the 
> multiplication operator?
> 
> Oz Linden wrote:
> I meant putting the constant being squared inside parens with no spaces.  
> Really, if it's a constant I think it's probably better to make another 
> constant with _SQUARED appended.
> 
> While I'm being nit-picky - I think it's clearer when wrapping an 
> expression across lines to put the operator at the beginning of the second 
> line instead of the end of the first line:
> 
> (dist_vec_squared(LLVector3(ki_prev->mPos), first_frame_pos) 
>  > (POSITION_MOTION_THRESHOLD*POSITION_MOTION_THRESHOLD))
>
> 
> Cron Stardust wrote:
> Oz, I was thinking along those lines (creating or changing the constant,) 
> but I wasn't sure if it would be within the scope of this.  After further 
> thought, and evaluating the input thus far, I've come to the conclusion that 
> it is within the scope.  A new diff incorporating the notes thus far 
> mentioned will be forthcoming.

Actually, it's not wrapped - that's a figment of your screen width. :)  
However, this is also now the old edition of the patch, the new revision 
changes POSITION_MOTION_THRESHOLD into POSITION_MOTION_THRESHOLD_SQUARED.


- Cron


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


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

Re: [opensource-dev] Review Request: Add optional range ring to the mini-map -- one centered on you with a radius of 20m to show local chat range

[Cron Stardust]

> On March 17, 2011, 4:50 a.m., Vadim ProductEngine wrote:
> > indra/newview/llnetmap.cpp, lines 473-474
> > 
> >
> > Please move these numbers to settings.

Those two should be CHAT_NORMAL_RADIUS and CHAT_SHOUT_RADIUS respectively.  
Both are defined in indra_constants.h in the llcommon directory.


- Cron


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


On March 12, 2011, 8 a.m., Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/197/
> ---
> 
> (Updated March 12, 2011, 8 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Add optional range ring to the mini-map -- one centered on you with a radius 
> of 20m to show local chat range.
> 
> By default the range ring is off.
> 
> To turn it on you right click on the mini-map and pick the menu entry "Range 
> Ring".
> 
> 
> This addresses bug Storm-1068.
> http://jira.secondlife.com/browse/Storm-1068
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt aed94e854443 
>   indra/newview/app_settings/settings.xml aed94e854443 
>   indra/newview/llnetmap.h aed94e854443 
>   indra/newview/llnetmap.cpp aed94e854443 
>   indra/newview/skins/default/colors.xml aed94e854443 
>   indra/newview/skins/default/xui/en/menu_mini_map.xml aed94e854443 
> 
> Diff: http://codereview.secondlife.com/r/197/diff
> 
> 
> Testing
> ---
> 
> Tested with another avatar.  When they are outside the ring they cannot see 
> my local chat and when they are inside it they can, so the ring's radius is 
> set correctly.
> 
> Flew over various types of land to make sure my color choice (blue @10%) was 
> always visible.
> 
> Enabled range ring, logged out and back on; range ring is still present on 
> mini-map.
> 
> Panned mini-map, range ring remains centered over avatar.
> 
> Used mouse wheel to zoom in and out as far as possible.  Size of range ring 
> varied with change in zoom level.
> 
> 
> Thanks,
> 
> Jonathan
> 
>

___
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

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

[Cron Stardust]

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

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

[Cron Stardust]

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


Changes
---

Switched to using *_SQUARED constants instead of multiplied constants, and 
cleaned up a few other minor issues noted during review.


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 (updated)
-

  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

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

[Cron Stardust]

> On March 12, 2011, 7:16 a.m., Oz Linden wrote:
> > indra/llcharacter/llbvhloader.cpp, line 1199
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1179#file1179line1199>
> >
> > I think it would be clearer to either add a new constant 
> > POSITION_MOTION_THRESHOLD_SQUARED or to write these like
> > 
> >  ... < (POSTION_MOTION_THRESHOLD*POSITION_MOTION_THRESHOLD).
> > 
> > (and same convention elsewhere)
> 
> Boroondas Gupte wrote:
> For this specific line, you probably mean ">", not "<", do you?
> 
> Also, what's the convention are you proposing? Grouping the multiplied 
> values together by braces? Or leaving away the spaces around the 
> multiplication operator?

Oz, I was thinking along those lines (creating or changing the constant,) but I 
wasn't sure if it would be within the scope of this.  After further thought, 
and evaluating the input thus far, I've come to the conclusion that it is 
within the scope.  A new diff incorporating the notes thus far mentioned will 
be forthcoming.


- Cron


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


On March 12, 2011, 6:33 a.m., Cron Stardust wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/199/
> ---
> 
> (Updated March 12, 2011, 6:33 a.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/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/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

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

[Cron Stardust]

> On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote:
> > indra/newview/llnetmap.cpp, line 337
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1187#file1187line337>
> >
> > Maybe add a short comment here, that this value is meant to be 
> > overwritten in the loop below it.

Good idea!  I'll do so.


> On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote:
> > indra/newview/llpanelpeople.cpp, lines 161-162
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1188#file1188line161>
> >
> > For clarity, please rename these variables to dist1_squared and 
> > dist2_squared, too. Or eliminate them by calling dist_vec_squared right in 
> > the return statement:
> > return dist_vec_squared(item1_pos, me_pos) < 
> > dist_vec_squared(item2_pos, me_pos);
> > 
> > (A bit long, but still shorter than the lines right above it.)

I think I'll go with the idea of remove the extraneous variables.  I don't 
think it makes the code that much less readable or understandable, and it 
leaves less for the compiler to have to optimize away. (Not that that last is a 
reason that can stand up on its own; I simply use it as spice for a stronger 
reason...)


> On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote:
> > indra/newview/llselectmgr.cpp, lines 6574-6587
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1189#file1189line6574>
> >
> > Memory reuse is good, I guess, but having variable names that only 
> > describe the variable's content correctly part of the time bothers me.
> 
> Oz Linden wrote:
> Agree with Boroondas here - the variable used inside the loop should be 
> min_dist_squared, not min_dist.
> 
> Leave the memory optimization to the compiler.

No problem.  I was just using what I had on hand. :)


> On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote:
> > indra/newview/llselectmgr.cpp, lines 6589-6594
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1189#file1189line6589>
> >
> > Off course, this variable already changed meaning during execution 
> > before your change, so ... meh.
> 
> Oz Linden wrote:
> but since you're touching it anyway, fix it

QSL: I'm on it! :D


- Cron


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


On March 12, 2011, 6:33 a.m., Cron Stardust wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/199/
> ---
> 
> (Updated March 12, 2011, 6:33 a.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/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/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

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

[Cron Stardust]

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

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 https://jira.secondlife.com/browse/VWR-25126.

http://jira.secondlife.com/browse/https://jira.secondlife.com/browse/VWR-25126


Diffs
-

  doc/contributions.txt 344d4c6d7d7e 
  indra/llcharacter/llbvhloader.cpp 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/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

[opensource-dev] Review Request: Corrected a call to the Mac-specific command "Rez" that had the wrong case.

[Cron Stardust]

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

Review request for Viewer.


Summary
---

Simple change from a lower-case "r" to an upper case "R" in the run_command 
call to the Rez command.

Should have no effect on case-insensitive Macs.  As this is, I believe, a 
Mac-specific command call, there should be no effect on other OSs.


This addresses bug VWR-25120.
http://jira.secondlife.com/browse/VWR-25120


Diffs
-

  doc/contributions.txt aed94e854443 
  indra/newview/viewer_manifest.py aed94e854443 

Diff: http://codereview.secondlife.com/r/196/diff


Testing
---

Built using a case-sensitive root partition on Mac OS 10.6


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

Re: [opensource-dev] Review Request: storm-1038: crash in texture cache pruning

[Cron Stardust]

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


As Thickbrick just noted on OSDev, this looks to be practically identical to 
https://codereview.secondlife.com/r/93

- Cron


On March 3, 2011, 12:12 p.m., Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/172/
> ---
> 
> (Updated March 3, 2011, 12:12 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> If the viewer is started when the maximum number of items allowed in the 
> texture cache is greater than the number actually in the cache, the viewer 
> crashes.
> 
> 
> This addresses bug storm-1038.
> http://jira.secondlife.com/browse/storm-1038
> 
> 
> Diffs
> -
> 
>   indra/newview/lltexturecache.cpp 8596ccc522d9 
> 
> Diff: http://codereview.secondlife.com/r/172/diff
> 
> 
> Testing
> ---
> 
> Since I found this by somehow getting into the over-full condition when 
> working on an unrelated issue, I found myself unable to launch my viewer.  
> One this fix was applied, launching worked just fine.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
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

Re: [opensource-dev] Review Request: storm-1038: crash in texture cache pruning

[Cron Stardust]

> On None, Oz Linden wrote:
> > indra/newview/lltexturecache.cpp, line 1439
> > 
> >
> > This was the crash - note that it is exactly the same condition that 
> > would normally get you out of the loop above.
> >

Any idea why this assert was there in the first place?  It looks to me like the 
code was escaping the loop on an error clause (which would explain why it was 
done in "if() break;" style,) and then asserting on that error.  If I'm right 
then why there wasn't any comments on those lines explaining the reasoning 
escapes me...  Such logic is far from obvious! :P

Note also that if there were no entries to purge (entries_to_purge < 0 because 
the above code found enough empty entries to successfully reduce the cache size 
enough,) and purge_list is empty (as it should be at this point in the code,) 
then the assert would fire.  Of course, I'm just trying to determine the 
reasoning behind the assert in the first place, and exercise my code reverse 
engineering skills. (The last few classes haven't had an ounce of real code, so 
I'm bored... :P )

PS: While you're in this territory, you might want to put some spaces around 
the subtraction operator inside the parenthesis on line 1426.  It looked too 
much like a dash to me on first glance!


- Cron


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


On March 3, 2011, 12:12 p.m., Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/172/
> ---
> 
> (Updated March 3, 2011, 12:12 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> If the viewer is started when the maximum number of items allowed in the 
> texture cache is greater than the number actually in the cache, the viewer 
> crashes.
> 
> 
> This addresses bug storm-1038.
> http://jira.secondlife.com/browse/storm-1038
> 
> 
> Diffs
> -
> 
>   indra/newview/lltexturecache.cpp 8596ccc522d9 
> 
> Diff: http://codereview.secondlife.com/r/172/diff
> 
> 
> Testing
> ---
> 
> Since I found this by somehow getting into the over-full condition when 
> working on an unrelated issue, I found myself unable to launch my viewer.  
> One this fix was applied, launching worked just fine.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
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

Re: [opensource-dev] Review Request: STORM-236 Actual Code Review

[Cron Stardust]

> On Jan. 21, 2011, 3:44 a.m., Boroondas Gupte wrote:
> > indra/newview/skins/default/xui/en/menu_bottomtray.xml, lines 11-13
> > 
> >
> > Begin XML comments with just "", not 
> > "".

Typically SGML/XML comments are supposed to start with "" (note the post/pre-fixed whitespace) - however, 
http://www.w3.org/TR/2008/REC-xml-20081126/#sec-comments does not require the 
space in the syntax, it only demonstrates it.


- Cron


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


On Jan. 20, 2011, 6:37 p.m., Wolfpup Lowenhar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/113/
> ---
> 
> (Updated Jan. 20, 2011, 6:37 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This allows the Speak Button to auto-hide for those that do not use Voice at 
> all.
> 
> 
> This addresses bug STORM-236.
> http://jira.secondlife.com/browse/STORM-236
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 9c7d543fd15d 
>   indra/newview/llbottomtray.h 9c7d543fd15d 
>   indra/newview/llbottomtray.cpp 9c7d543fd15d 
>   indra/newview/llspeakbutton.cpp 9c7d543fd15d 
>   indra/newview/skins/default/xui/en/menu_bottomtray.xml 9c7d543fd15d 
> 
> Diff: http://codereview.secondlife.com/r/113/diff
> 
> 
> Testing
> ---
> 
> Built locally and did the following:
> 1 Verified that when Voice is toggled via the preference panel the Speak 
> Button auto hid/showed.
> 2 Verified that drag and drop functionality of the Speak Button was not 
> affected.
> 3 Went to a non-Voice area with Voice active and verified that button was 
> still there but grayed out.
> 
> 
> Thanks,
> 
> Wolfpup
> 
>

___
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

Re: [opensource-dev] Review Request: Help Needed to debug small problem with code for STORM-236.

[Cron Stardust]

> On Jan. 20, 2011, 10:45 a.m., Cron Stardust wrote:
> > Looks like you are only preventing the button from drawing and working, but 
> > not actually /removing/ it from the tray.  Have you hunted down the code 
> > that is used to remove tray buttons when the tray is right-clicked on and 
> > an item is checked/unchecked?  I would think that that is the functionality 
> > that needs to be copied onto the speak button.
> > 
> > Maybe, as a different JIRA entry and review, you might consider what it 
> > would take to add the speak button to that tray context menu.  Once the 
> > ability to remove and add the speak button via the context menu is working, 
> > it would then seem to me that this entry (STORM-236) would be trivial to 
> > implement, as all the core functionality will have been resolved already.  
> > Even if the new entry for the context menu wasn't accepted, the core 
> > functionality can still be done here.
> 
> Wolfpup Lowenhar wrote:
> This Issue is for having the button hide(not be drawn) and free up the 
> space that it normally take in the bottom tray. The problem I'm having is 
> with the second part getting the space freed up right. Also the context menu 
> is functional so there are no problems there.
> 
> Cron Stardust wrote:
> As of 2.6.0.219259 the Speak button is not able to be removed from the 
> tray using the context menu.  However, the point I was trying to make was 
> that the underlying functionality of what you are trying to accomplish here, 
> that of removing a button and freeing up the space, is currently accomplished 
> by the underlying functionality that the tray context menu taps into.  
> Whether a simple (hah!) study of that code's functionality will give you what 
> you need, or whether you do a complementary implementation of adding the 
> (de)activation of the Speak button to the context menu, is up to you.
> 
> I simply was pointing out that the underlying task is accomplished at 
> another, potentially missed, location.  If you have already studied and 
> implemented the button (de)activation code that the tray context menu uses, 
> then my suggestion is just noise.  On the other hand, exploring what it would 
> take to make the Speak button go away or come back via the context menu 
> allows you to explore another code section that might result in two birds 
> being killed with one stone: making the button removable for those who only 
> use the keyboard or mouse toggle, and giving you the code to make the button 
> go away when Voice isn't active.
> 
> Hope that clears up the situation.  To me the problem here isn't the 
> tasks of "make the button hide" and then "free up the space" - it's about how 
> to properly remove and re-add the button, similar to how the tray context 
> menu works, just with a different trigger: the voice setting.
> 
> Wolfpup Lowenhar wrote:
> Cron,
>  I wonder if you have even looked at the diff as im working on adding all 
> the functionality that you say is not present. Yes there is some thing im 
> missing other wise I would not be receiving an error in my local repository. 
> this work is only in my local repository at present as it is not yet ready to 
> be posted for review but as Oz suggested i placed this here with the subject 
> "Help Needed" as i believe im missing some thing important in the code and 
> there for getting a error. This is ACTUALLY a request for ASSISTANCE from the 
> open source community and not a review request.

Wolfpup, I am trying to give assistance.  Just like I assist my fellow CS 
students in the programming labs when I'm not familiar with the details of what 
they are implementing.  Yes, I have looked over the diff.  Yet since I have 
never gotten a build environment that worked for more than a day, I didn't have 
the time (back when I did have the time) to learn the ins and outs of the 
viewer codebase.  Since then I've not had the time to do more than glance over 
other people's code.  Hopefully I will learn enough from that to start working 
changes myself when I have time again.  Since I don't have a build env, I can't 
help with straight debugging.  However, I believe that my points and 
information may be valid and useful in helping direct your mind to a solution, 
even though I cannot supply one.

Often when I have a coding problem, I'll even go so far as to explain it to an 
artist: their eyes may glaze over, but in their ignorance of how things work in 
programming sometimes makes them able to point out areas to explore or 
solutions that I never would have thought of.  Such is what I'm trying to do 
here: provide you with a way around the barrie

Re: [opensource-dev] Review Request: Help Needed to debug small problem with code for STORM-236.

[Cron Stardust]

> On Jan. 20, 2011, 10:45 a.m., Cron Stardust wrote:
> > Looks like you are only preventing the button from drawing and working, but 
> > not actually /removing/ it from the tray.  Have you hunted down the code 
> > that is used to remove tray buttons when the tray is right-clicked on and 
> > an item is checked/unchecked?  I would think that that is the functionality 
> > that needs to be copied onto the speak button.
> > 
> > Maybe, as a different JIRA entry and review, you might consider what it 
> > would take to add the speak button to that tray context menu.  Once the 
> > ability to remove and add the speak button via the context menu is working, 
> > it would then seem to me that this entry (STORM-236) would be trivial to 
> > implement, as all the core functionality will have been resolved already.  
> > Even if the new entry for the context menu wasn't accepted, the core 
> > functionality can still be done here.
> 
> Wolfpup Lowenhar wrote:
> This Issue is for having the button hide(not be drawn) and free up the 
> space that it normally take in the bottom tray. The problem I'm having is 
> with the second part getting the space freed up right. Also the context menu 
> is functional so there are no problems there.

As of 2.6.0.219259 the Speak button is not able to be removed from the tray 
using the context menu.  However, the point I was trying to make was that the 
underlying functionality of what you are trying to accomplish here, that of 
removing a button and freeing up the space, is currently accomplished by the 
underlying functionality that the tray context menu taps into.  Whether a 
simple (hah!) study of that code's functionality will give you what you need, 
or whether you do a complementary implementation of adding the (de)activation 
of the Speak button to the context menu, is up to you.

I simply was pointing out that the underlying task is accomplished at another, 
potentially missed, location.  If you have already studied and implemented the 
button (de)activation code that the tray context menu uses, then my suggestion 
is just noise.  On the other hand, exploring what it would take to make the 
Speak button go away or come back via the context menu allows you to explore 
another code section that might result in two birds being killed with one 
stone: making the button removable for those who only use the keyboard or mouse 
toggle, and giving you the code to make the button go away when Voice isn't 
active.

Hope that clears up the situation.  To me the problem here isn't the tasks of 
"make the button hide" and then "free up the space" - it's about how to 
properly remove and re-add the button, similar to how the tray context menu 
works, just with a different trigger: the voice setting.


- Cron


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


On Jan. 20, 2011, 9:29 a.m., Wolfpup Lowenhar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/112/
> ---
> 
> (Updated Jan. 20, 2011, 9:29 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This is actually a request for for help to debug what I have done so far to 
> get this working.
> 1. In the current diff the button dose actualy hide and show according to if 
> Voice is enabled or not.
> 2. i have two problems at this point.
>a. the space taken up by the speak button is not freed up
>b. when the speak button is shown after being hidden there is an error 
> message saying no space avaiable
>   (I think this is related to the fact that the space is not freed to 
> begin with).
> 
> 
> This addresses bug STORM-236.
> http://jira.secondlife.com/browse/STORM-236
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 40d0806e9800 
>   indra/newview/llbottomtray.cpp 40d0806e9800 
>   indra/newview/llspeakbutton.cpp 40d0806e9800 
>   indra/newview/skins/default/xui/en/menu_bottomtray.xml 40d0806e9800 
> 
> Diff: http://codereview.secondlife.com/r/112/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Wolfpup
> 
>

___
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

Re: [opensource-dev] Review Request: Help Needed to debug small problem with code for STORM-236.

[Cron Stardust]

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


Looks like you are only preventing the button from drawing and working, but not 
actually /removing/ it from the tray.  Have you hunted down the code that is 
used to remove tray buttons when the tray is right-clicked on and an item is 
checked/unchecked?  I would think that that is the functionality that needs to 
be copied onto the speak button.

Maybe, as a different JIRA entry and review, you might consider what it would 
take to add the speak button to that tray context menu.  Once the ability to 
remove and add the speak button via the context menu is working, it would then 
seem to me that this entry (STORM-236) would be trivial to implement, as all 
the core functionality will have been resolved already.  Even if the new entry 
for the context menu wasn't accepted, the core functionality can still be done 
here.

- Cron


On Jan. 20, 2011, 9:29 a.m., Wolfpup Lowenhar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/112/
> ---
> 
> (Updated Jan. 20, 2011, 9:29 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This is actually a request for for help to debug what I have done so far to 
> get this working.
> 1. In the current diff the button dose actualy hide and show according to if 
> Voice is enabled or not.
> 2. i have two problems at this point.
>a. the space taken up by the speak button is not freed up
>b. when the speak button is shown after being hidden there is an error 
> message saying no space avaiable
>   (I think this is related to the fact that the space is not freed to 
> begin with).
> 
> 
> This addresses bug STORM-236.
> http://jira.secondlife.com/browse/STORM-236
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 40d0806e9800 
>   indra/newview/llbottomtray.cpp 40d0806e9800 
>   indra/newview/llspeakbutton.cpp 40d0806e9800 
>   indra/newview/skins/default/xui/en/menu_bottomtray.xml 40d0806e9800 
> 
> Diff: http://codereview.secondlife.com/r/112/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Wolfpup
> 
>

___
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

Re: [opensource-dev] Review Request: VWR-24321: Validate textures starting with 00 too.

[Cron Stardust]

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

Ship it!


Thanks for the clarification!  Looks good to me.

- Cron


On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/90/
> ---
> 
> (Updated Jan. 14, 2011, 1:02 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Trivial patch, just removes stupidity.
> 
> 
> This addresses bug VWR-24321.
> http://jira.secondlife.com/browse/VWR-24321
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt b0bd26c5638a 
>   indra/newview/lltexturecache.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/90/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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

Re: [opensource-dev] Review Request: VWR-24321: Validate textures starting with 00 too.

[Cron Stardust]

> On Jan. 18, 2011, 12:09 p.m., Merov Linden wrote:
> > indra/newview/lltexturecache.cpp, line 1595
> > 
> >
> > validate_idx being used in a test later, it's not just for 
> > (validate_idx == 0) that the behavior will be different. I need to 
> > understand better what that idx is all about or you need to give a bit more 
> > explanation before I approve this diff.
> 
> Aleric Inglewood wrote:
> The debug setting CacheValidateCounter is set to 'next_id', which makes 
> clear what it's meaning is: namely, the id that we will check next time. 
> next_idx is a very local variable that is simply set to the value of 
> CacheValidateCounter plus 1, and then that value is stored to 
> CacheValidateCounter again for next time.
> 
> validate_idx is the ID that is actually being tested this time. Hence, it 
> should be the value of CacheValidateCounter before we increase that.
> 
> Obviously, those ID's should be in the range 0...255, which is garanteed 
> by doing a modulo 256 on next_id before writing it to CacheValidateCounter.
> 
> The old code also increases validate_idx *before* it is used. That means 
> that it will be in the range 1...256, and 0 is never tested (note that 
> validate_idx is just increased, there is no modulo applied, so it's obvious 
> that it shouldn't be increased). Even the wording ("next"_id) suggests that 
> validate_idx should just be the value of CacheValidateCounter, which is the 
> value of the previous next_id...
> 
> So, after this patch, we get to the following code with validate_idx in 
> the range 0...255, as it should be.
>

Just double checking, as switching from pre-increment to addition can change 
behavior: In both cases next_idx will have the same value after this line is 
executed, however validate_idx will not.

Prediff start state: validate_idx == 0;
Prediff stop state: validate_idx == 1; next_idx == 1;

Postdiff start state: validate_idx == 0;
Postdiff stop state: validate_idx == 0; next_idx == 1;

And another round over at the other end:

Prediff start state: validate_idx == 255;
Prediff stop state: validate_idx == 256; next_idx == 0;

Postdiff start state: validate_idx == 255;
Postdiff stop state: validate_idx == 255; next_idx == 0;

So, yes, validate_idx will only have a 255 in it in this last case, however the 
over-all behavior has changed: validate_idx isn't being incremented at all in 
this line.

WARNING: I have NOT looked at the rest of the diff, only at this one line.  Nor 
do I know if validate_idx should or shouldn't be incremented by this line of 
code.


- Cron


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


On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/90/
> ---
> 
> (Updated Jan. 14, 2011, 1:02 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Trivial patch, just removes stupidity.
> 
> 
> This addresses bug VWR-24321.
> http://jira.secondlife.com/browse/VWR-24321
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt b0bd26c5638a 
>   indra/newview/lltexturecache.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/90/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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

Re: [opensource-dev] Review Request: STORM-34 As a User, I want a list of my favorite locations available on the login screen so i can log in to SL right where I want to be.

[Cron Stardust]

> On 2010-12-06 08:33:21, Seth ProductEngine wrote:
> > Looks good for a first version of the task implementation, considering the 
> > issues mentioned in the description.

Agreed.  The code looks clean. (As expected from the pro's! :D )  Thought I had 
found a style issue, but double checked the style guide (no guidance found 
there,) and then checked similar code and found the same style.  So no problem. 
:)


- Cron


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


On 2010-12-06 08:00:40, Andrew ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/4/
> ---
> 
> (Updated 2010-12-06 08:00:40)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Saving of user's favorites into file and showing them in "Start at" combobox 
> on login screen was implemented.
> 
> 
> Implementation details:
> 
> - File is saved on exit from viewer and not immediately on changes as was 
> written in spec. It is done to make this file consistent with favorites 
> order: order of favorites is saved on exit,
> so if favorites info is saved in other moment earlier, crashing viewer or 
> other unexpected way of finishing its work (i.e. via Windows task bar) would 
> cause inconsistence between favorites order
> saved per account and one from this new file.
> 
> - File is saved in user_settings\stored_favorites.xml.
> 
> - If you uncheck the option in Preferences and press OK, the file gets 
> immediately deleted (according to spec).
> 
> 
> Issues that require further changes:
> 
> - Currently only favorites of last logged in user are shown in login screen. 
> Showing favorites of multiple users will be implemented later when design for 
> it is approved by Esbee.
> 
> - Preference is now global for all users, because design states it may be 
> changed before login, and we don't have account info at the moment. But it 
> doesn't seem to be a good idea, so changes in design are needed.
> 
> - Currently the way of retrieving SLURLs needs optimization in a separate 
> ticket.
> 
> More detailed design approved by Esbee is needed to develop it further, 
> perhaps in new tickets.
> 
> 
> This addresses bug storm-34.
> http://jira.secondlife.com/browse/storm-34
> 
> 
> Diffs
> -
> 
>   indra/newview/app_settings/settings.xml e843e274fa58 
>   indra/newview/llfavoritesbar.cpp e843e274fa58 
>   indra/newview/llfloaterpreference.h e843e274fa58 
>   indra/newview/llfloaterpreference.cpp e843e274fa58 
>   indra/newview/llpanellogin.h e843e274fa58 
>   indra/newview/llpanellogin.cpp e843e274fa58 
>   indra/newview/llviewerinventory.cpp e843e274fa58 
>   indra/newview/skins/default/xui/en/notifications.xml e843e274fa58 
>   indra/newview/skins/default/xui/en/panel_preferences_privacy.xml 
> e843e274fa58 
> 
> Diff: http://codereview.secondlife.com/r/4/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew
> 
>

___
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

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Cron Stardust]

> On 2010-12-01 21:26:12, Cron Stardust wrote:
> > Based on the logic of the removed break, line 1192 of the fixed file:
> >  idleTimer.getElapsedTimeF64() >= max_idle_time
> > should be
> >  idleTimer.getElapsedTimeF64() < max_idle_time
> > 
> > The variable "S32 pending;" is redeclared on line 1613 of the fixed file.  
> > The previous edition didn't have this issue due to scoping.
> > 
> > Looks like the break on 4224 can't be easily removed: it's a tail test in a 
> > loop that seems to be required to be a head-test loop.  Maybe the logic and 
> > purpose can be reanalyzed, but then this would be more than a refactor.
> 
> Wolfpup Lowenhar wrote:
> we are just testing here :p

True, but reporting errors (and getting them fixed) is as much part of the test 
(IMHO) as everything else. :D


- Cron


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


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
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

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Cron Stardust]

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


Based on the logic of the removed break, line 1192 of the fixed file:
 idleTimer.getElapsedTimeF64() >= max_idle_time
should be
 idleTimer.getElapsedTimeF64() < max_idle_time

The variable "S32 pending;" is redeclared on line 1613 of the fixed file.  The 
previous edition didn't have this issue due to scoping.

Looks like the break on 4224 can't be easily removed: it's a tail test in a 
loop that seems to be required to be a head-test loop.  Maybe the logic and 
purpose can be reanalyzed, but then this would be more than a refactor.

- Cron


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> Thanks,
> 
> Oz
> 
>

___
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