> 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/ > > Seth ProductEngine wrote: > Changed "is: to "if". Is that correct?
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? > > Seth ProductEngine wrote: > 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? Ah, I thought assets would only be removed from the list when they finished loading. If it were so, someone always triggering a new gesture (with new assets) before all previously queued assets have been loaded could thus prevent the list from becoming empty and thereby delay playback of all gestures forever, even although assets for most gestures would already be available. This could happen even in the case of no assets failing to load. Emptying the list (is that line 532 in the new indra/newview/llgesturemgr.cpp ?) when a new gesture comes in avoids this. Off course it isn't really the Right Thing To Do(TM), but if the behavior will in no case be worse than the current code, we can go for this until pending downloads are checked separately for each queued gesture. The latter can probably be achieved by maintaining a separate list of pending assets for each gesture and start playing a gesture when its own list has become empty. > 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. > > Seth ProductEngine wrote: > 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. Is this already checked somewhere before here, or could a bogus gesture trigger that assert? If the latter, a clean-up step and some terminal output would be friendlier than an assert, as we don't want the ability to 'shoot down' a Viewer by providing invalid data from remote. - Boroondas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/231/#review506 ----------------------------------------------------------- On March 28, 2011, 12:21 p.m., Seth ProductEngine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/231/ > ----------------------------------------------------------- > > (Updated March 28, 2011, 12:21 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 b3cfba00a29b > indra/newview/llgesturemgr.cpp b3cfba00a29b > > 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
