> On March 26, 2011, 8:55 a.m., Boroondas Gupte wrote: > > indra/newview/llgesturemgr.h, lines 161-164 > > <http://codereview.secondlife.com/r/231/diff/1/?file=1326#file1326line161> > > > > Please use tabs for indentation and spaces for alignment. (Here, that'd > > be only one tab followed by 32 spaces.) Whatever you did here looks wrong > > whether I set tab display length to 4 or 8.
Corrected indentation here and for the method declared above. > On March 26, 2011, 8:55 a.m., Boroondas Gupte wrote: > > indra/newview/llgesturemgr.cpp, lines 536-537 > > <http://codereview.secondlife.com/r/231/diff/1/?file=1327#file1327line536> > > > > If the iterator will not be used outside the loop, please declare it > > with the assignment in the for's braces. > > Oz Linden wrote: > I don't see any reason to make that distinction. Seems like it may look confusing if the iterator is not used outside the loop, so moved its declaration inside braces. > On March 26, 2011, 8:55 a.m., Boroondas Gupte wrote: > > indra/newview/llgesturemgr.cpp, line 549 > > <http://codereview.secondlife.com/r/231/diff/1/?file=1327#file1327line549> > > > > I'm not sure what you mean by "if it stops". Maybe that the currently > > handled gesture step stops the animation? If so, maybe write > > > > // Don't request the animation if this step stops it or if it is > > already in Static VFS Corrected. > On March 26, 2011, 8:55 a.m., Boroondas Gupte wrote: > > indra/newview/llgesturemgr.cpp, lines 582-587 > > <http://codereview.secondlife.com/r/231/diff/1/?file=1327#file1327line582> > > > > Why are STEP_CHAT and STEP_WAIT mentioned when they don't do anything > > and we do have a default step that also doesn't do anything? > > > > If all cases are supposed to be explicitly handled, we might want to > > make sure of that with: > > // [...] > > case STEP_CHAT: > > case STEP_WAIT: > > { > > break; > > } > > default: > > { > > // We should never get here. > > llassert(false); > > } > > } > > > > (... or maybe just some terminal output.) Added the STEP_EOF for a complete list of step types and a warning for the default case. > On March 26, 2011, 8:55 a.m., Boroondas Gupte wrote: > > indra/newview/llgesturemgr.cpp, lines 763-764 > > <http://codereview.secondlife.com/r/231/diff/1/?file=1327#file1327line763> > > > > s/is/if/ Changed "is: to "if". Is that correct? > On March 26, 2011, 8:55 a.m., Boroondas Gupte wrote: > > indra/newview/llgesturemgr.cpp, lines 765-766 > > <http://codereview.secondlife.com/r/231/diff/1/?file=1327#file1327line765> > > > > Hmm ... so with the current change, a gesture's playback could > > potentially be delayed forever due to assets for other gestures being > > loaded? For now starting each new gesture resets the list of pending assets downloads so if the last started gesture's asset are loaded successfully the playback will continue. This should probably need a better way to handle the cases of some assets not being loaded due to some kind of error. The current behavior is not suppressing the gesture playback due to any data loading delays. Should we follow this behavior? > On March 26, 2011, 8:55 a.m., Boroondas Gupte wrote: > > indra/newview/llgesturemgr.cpp, lines 1194-1197 > > <http://codereview.secondlife.com/r/231/diff/1/?file=1327#file1327line1194> > > > > Can a gesture reference other asset types than animations and sounds? > > And if it can, shouldn't those be removed from mLoadingAssets, too, once > > loaded? > > > > If it can but shouldn't maybe some terminal output would be appropriate > > here. It shouldn't reference other asset types and there are specific callback procedures to handle animation and sound assets so added an assert for the default case. - Seth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/231/#review506 ----------------------------------------------------------- On March 25, 2011, 5:33 p.m., Seth ProductEngine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/231/ > ----------------------------------------------------------- > > (Updated March 25, 2011, 5:33 p.m.) > > > Review request for Viewer. > > > Summary > ------- > > First pass implementation of syncing the animations and sounds before the > gesture starts playing. > The actual playing of animations and sounds of a gesture starts only when all > needed animations and sound files are loaded into viewer cache. This reduces > the delay between animations and sounds meant to be played simultaneously but > may increase the delay between the moment a gesture is triggered and the > moment it starts playing. > > > This addresses bug STORM-380. > http://jira.secondlife.com/browse/STORM-380 > > > Diffs > ----- > > indra/newview/llgesturemgr.h 6c15f820c3b9 > indra/newview/llgesturemgr.cpp 6c15f820c3b9 > > Diff: http://codereview.secondlife.com/r/231/diff > > > Testing > ------- > > > Thanks, > > Seth > >
_______________________________________________ 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
