[opensource-dev] Review Request: STORM-64: Local Bitmaps 3.0 implementation.

8 views
Skip to first unread message

Vaalith Jinn

unread,
Mar 23, 2012, 12:01:02 PM3/23/12
to Vaalith Jinn, Viewer
This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/566/

Review request for Viewer.
By Vaalith Jinn.

Description 

Local Bitmaps is a mechanism to locally load images into the viewer, track them and optionally (per each image)
have it check if the image has been overwritten locally and if so - update it in the viewer and inworld.

This change affects the texture picker - adding radio checks that let the user choose between the "Inventory" (regular inventory) and "Local" tabs (list of locally added files).

*** DO NOT USE THIS YET ***
do not implement for live viewers until fully reviewed.
Bugs: STORM-64

Diffs

  • doc/contributions.txt (96a53bafcd1c)
  • indra/newview/CMakeLists.txt (96a53bafcd1c)
  • indra/newview/lllocalbitmaps.h (PRE-CREATION)
  • indra/newview/lllocalbitmaps.cpp (PRE-CREATION)
  • indra/newview/lltexturectrl.h (96a53bafcd1c)
  • indra/newview/lltexturectrl.cpp (96a53bafcd1c)
  • indra/newview/llviewertexturelist.h (96a53bafcd1c)
  • indra/newview/llwearable.h (96a53bafcd1c)
  • indra/newview/llwearable.cpp (96a53bafcd1c)
  • indra/newview/skins/default/xui/en/floater_texture_ctrl.xml (96a53bafcd1c)

View Diff

Oz Linden

unread,
Mar 28, 2012, 12:58:30 PM3/28/12
to Oz Linden, Vaalith Jinn, Viewer
This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/566/ 

A couple of things to look at, but overall very very nice looking code.

indra/newview/lllocalbitmaps.h (Diff revision 1) 

   
class LLLocalBitmap


  

 




 
 



 

  

    
    

    42
    
		bool updateSelf(bool first_update = false);

  


 





Minor point ... it would be clearer to have the parameter be an enum whose values were first_use and texture_updated or something.  

Just because something only has two values doesn't make it a 'bool'.














 
  

   
    indra/newview/lllocalbitmaps.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   

   
bool LLLocalBitmap::updateSelf(bool first_update)


  

 




 
 



 

  

    
    

    230
    
						mLinkStatus = LS_BROKEN;

  


 





Would an LL_WARN be appropriate here?














 
  

   
    indra/newview/lllocalbitmaps.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   

   
bool LLLocalBitmap::updateSelf(bool first_update)


  

 




 
 



 

  

    
    

    250
    
	switch (mExtension)

  


  

    
    

    251
    
	{

  


  

    
    

    252
    
		case ET_IMG_BMP:

  


  

    
    

    253
    
		{

  


  

    
    

    254
    
			LLPointer<LLImageBMP> bmp_image = new LLImageBMP;

  


  

    
    

    255
    
			if (bmp_image->load(mFilename) && bmp_image->decode(rawimg, 0.0f))

  


  

    
    

    256
    
			{

  


  

    
    

    257
    
				rawimg->biasedScaleToPowerOfTwo(LLViewerFetchedTexture::MAX_IMAGE_SIZE_DEFAULT);

  


  

    
    

    258
    
				decode_successful = true;

  


  

    
    

    259
    
				break;

  


  

    
    

    260
    
			}

  


  

    
    

    261
    
		}

  


 





The cases in this switch all fall through if something unexpected happens... and there's no code to handle the unexpected.

If falling through is intentional, there should be a comment saying so, and if not the break should follow the end of the (missing) else block.














 
  

   
    indra/newview/lllocalbitmaps.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   

   
bool LLLocalBitmap::updateSelf(bool first_update)


  

 




 
 



 

  

    
    

    368
    
				for(; objlist_iter != obj_list.end(); objlist_iter++)

  


 





you could shorten this loop by adding ' && add_object ' to the termination condition














 
  

   
    indra/newview/lllocalbitmaps.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   

   
bool LLLocalBitmap::updateSelf(bool first_update)


  

 




 
 



 

  

    
    

    395
    
	std::vector<LLViewerObject*>::iterator object_iterator = objectlist.begin();

  


 





clearer to put the assignment in the first clause of the for loop














 
  

   
    indra/newview/lllocalbitmaps.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   

   
bool LLLocalBitmap::updateSelf(bool first_update)


  

 




 
 



 

  

    
    

    493
    
	switch(type)

  


 





I'd prefer a single 'return result;' at the bottom, replacing all those scattered ones with 'break;' because it's easier to set breakpoints on.









- Oz






On March 23rd, 2012, 9:01 a.m., Vaalith Jinn wrote:











Review request for Viewer.


By Vaalith Jinn.




Updated March 23, 2012, 9:01 a.m.
Oz Linden
unread,
Mar 28, 2012, 12:59:12 PM3/28/12
to Callum Linden, Richard Nelson, Viewer

   

    

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

   

   





 

  


Review request for Viewer, Callum Linden and Richard Nelson.


By Vaalith Jinn.




Updated March 28, 2012, 9:59 a.m.
Vaalith Jinn
unread,
Mar 29, 2012, 8:12:52 AM3/29/12
to Callum Linden, Richard Nelson, Viewer

   

    

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

   

   







Review request for Viewer, Callum Linden and Richard Nelson.


By Vaalith Jinn.




Updated March 29, 2012, 5:12 a.m.





Changes



 

  
   
Included fd797a815c0a STORM-64: small fix for building on linux.
Included 3e3ba231afb0 STORM-64: various fixes pointed out at codereview.

Both of these already in the https://bitbucket.org/serpentu/storm-64-new repo. 

  		
 








Description 

 

 

  
   
Local Bitmaps is a mechanism to locally load images into the viewer, track them and optionally (per each image)
have it check if the image has been overwritten locally and if so - update it in the viewer and inworld.

This change affects the texture picker - adding radio checks that let the user choose between the "Inventory" (regular inventory) and "Local" tabs (list of locally added files).

*** DO NOT USE THIS YET ***
do not implement for live viewers until fully reviewed.

  		
 









 Bugs: 


 STORM-64







Diffs (updated)
Vaalith Jinn
unread,
Mar 29, 2012, 8:28:11 AM3/29/12
to Callum Linden, Richard Nelson, Viewer

   

    

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

   

   


















 
  

   
    indra/newview/lllocalbitmaps.h
    

     (Diff revision 1)

    
   
  

 

 
  


   

   
class LLLocalBitmap


  

 




 
 



 

  

    
    

    42
    
		bool updateSelf(bool first_update = false);

  


 





For clarity's sake i kept private enums block below where it's currently at.

If i were to add an enum value in here i'd have to either move the whole block up to keep consistency or break consistency and write in a single enum below the main public block..

Neither is a really pretty solution as far as readability is concerned.

Is it really worth it?









- Vaalith






On March 29th, 2012, 5:12 a.m., Vaalith Jinn wrote:











Review request for Viewer, Callum Linden and Richard Nelson.


By Vaalith Jinn.




Updated March 29, 2012, 5:12 a.m.








Description 

 

 

  
   
Local Bitmaps is a mechanism to locally load images into the viewer, track them and optionally (per each image)
have it check if the image has been overwritten locally and if so - update it in the viewer and inworld.

This change affects the texture picker - adding radio checks that let the user choose between the "Inventory" (regular inventory) and "Local" tabs (list of locally added files).

*** DO NOT USE THIS YET ***
do not implement for live viewers until fully reviewed.

  		
 









 Bugs: 


 STORM-64







Diffs 


Oz Linden
unread,
Mar 30, 2012, 10:39:31 AM3/30/12
to Callum Linden, Richard Nelson, Viewer

   

    

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

   

   


















 
  

   
    indra/newview/lllocalbitmaps.cpp
    

     (Diff revision 2)

    
   
  

 

 
  


   

   
LLVOAvatarDefines::ETextureIndex LLLocalBitmap::getTexIndex(


  

 




 
 



 

  

    
    

    554
    

  


 





Need 'break;' here.














 
  

   
    indra/newview/lllocalbitmaps.cpp
    

     (Diff revision 2)

    
   
  

 

 
  


   

   
LLVOAvatarDefines::ETextureIndex LLLocalBitmap::getTexIndex(


  

 




 
 



 

  

    
    

    648
    
		}

  


 





Need 'break;' here.














 
  

   
    indra/newview/lllocalbitmaps.cpp
    

     (Diff revision 2)

    
   
  

 

 
  


   

   
LLVOAvatarDefines::ETextureIndex LLLocalBitmap::getTexIndex(


  

 




 
 



 

  

    
    

    697
    
		}

  


 





Need 'break;' here.














 
  

   
    indra/newview/lllocalbitmaps.cpp
    

     (Diff revision 2)

    
   
  

 

 
  


   

   
LLVOAvatarDefines::ETextureIndex LLLocalBitmap::getTexIndex(


  

 




 
 



 

  

    
    

    721
    
		}

  


 





log the fact that it's an unknown wearable type here?









- Oz






On March 29th, 2012, 5:12 a.m., Vaalith Jinn wrote:











Review request for Viewer, Callum Linden and Richard Nelson.


By Vaalith Jinn.




Updated March 29, 2012, 5:12 a.m.








Description 

 

 

  
   
Local Bitmaps is a mechanism to locally load images into the viewer, track them and optionally (per each image)
have it check if the image has been overwritten locally and if so - update it in the viewer and inworld.

This change affects the texture picker - adding radio checks that let the user choose between the "Inventory" (regular inventory) and "Local" tabs (list of locally added files).

*** DO NOT USE THIS YET ***
do not implement for live viewers until fully reviewed.

  		
 









 Bugs: 


 STORM-64







Diffs 


Vaalith Jinn
unread,
Mar 30, 2012, 7:06:11 PM3/30/12
to Callum Linden, Richard Nelson, Viewer

   

    

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

   

   







Review request for Viewer, Callum Linden and Richard Nelson.


By Vaalith Jinn.




Updated March 30, 2012, 4:06 p.m.





Changes



 

  
   
5790e41deb9e STORM-64: Additional fixes from codereview.

  		
 


Reply all
Reply to author
Forward
0 new messages