> On 2010-12-20 14:40:41, Nyx Linden wrote:
> > I have no technical objections to the code provided.
> > And in fact, the code provided *should* change the functionality back to
> > what the users are reporting is their expectation of what the behavior
> > should be.
> >
> > The part that makes me nervous is that this enables an outfit operation for
> > a class of folders that we have not allowed with the new outfit system
> > previously. Based on other pieces on the code that would get called by this
> > operation, I believe that the result will be that of the resident request
> > to "revert" behavior (namely remove all clothing, remove body parts that
> > are replaced by the new folder, leave old body parts that are necessary to
> > display the avatar).
> >
> > That being said, we need a more comprehensive review of the role of
> > incomplete outfits and how it fits with our technical architecture we've
> > built up in our current outfits system. The code here should implement the
> > correct behavior and I have no technical issues with it. But I want to make
> > sure that we are aware of the risk of edge cases as we have not considered
> > possibly popping up as a result of this patch.
My (mathematical) analysis of the "outfit system"
=================================================
- The outfit system exists of sets and operators.
- Each operator works on two sets, where both sets are an input and one of the
sets is used as output; this would be irrelevant for what the operators do
except when those operators are not commutative (which is be Bad Thing, but
unfortunately they are not). If one of those operators is written as '+'
(whatever that does), then for now we can speak about a + b, which doesn't
imply that this is the same as b + a and which intuitively extends to the
definition a += b ==> a = a + b. Note that the '+' operator is abritrary,
replace + with - and the same story holds.
- The elements of each set do not have the same type, and if they don't then an
operator can't work on them. Therefore, the correct way to look at these sets
is as vectors where an absent type will be represented by '0' (zero). That is
an arbitrary choice, but intuitive because our operators will be a lot more
like groups than like fields. Of course, this implies that a + 0 = 0 + a = a.
- Our vector (V) has one element per type. The types are: shape, (Linden) hair,
(Linden) shoes, eyes, skin, alpha, tatoes, undershirts, shirts, jackets,
gloves, undies, pants, socks, skull attachments, nose attachments, etc (one
type for each different attachment point).
- The elements of this vector are sets: for some types it is possible to wear
multiple of the same type at the same time. Note that these are sets, not
vectors: the order in which these wearables are added SHOULD be irrelevant (at
the moment it isn't; for example, old viewers only show the first attachment
that was attached, so the order is important).
- Some types may only have a single element (in their set) at a time. As a
result their operators are clearly not commutative, that is impossible. These
types are: shape, hair, shoes, eyes, skin and alpha (I think; the exact list is
irrelevant at this point). Lets call this class of types: S (of 'Single').
- The other types can be divided into two classes: wearables (tatoes,
(under)shirts, jackets, gloves, pants, socks) and attachments (skull, nose,
...). For an intuitive functioning of the outfit system these SHOULD behave the
same mathematically, but at this point we can't be certain if they do. However,
it will be clear that all wearables and all attachments respectively behave the
same. Lets call these classes W and A. [Note that V doesn't have to be
implemented as a linear vector, I think it would be better to implement it as a
vector of three vectors: one vector for elements of class S, one for elements
of class W and one for elements of class A.]
- For the S class we CAN have the following operations.
Let x and y be two distinct elements of the same type, where the type is of
class S.
[Note that for the Current Outfit the empty set is not allowed.]
Picking two arbitrary characters (+ and -) we can write for the possible
operators: x + y = y, x - y = x.
Other operators do not exist, but if they do out of necessity (namely, these
are elements of a vector and we will need to define operators of the vector,
which then in turn work on all types), then we can choose to let them be
equivalent to either + or -. Also note that this choice defines our extended
operators += and -= as: x += y ==> x = y, and x -= y ==> nul operator.
- For the A class we CAN have the following operations.
Let a and b be sets. Note that any distinct element can only be once in a
set: { e1 } + { e1 } = { e1 }, but { e1 } - { e1 } = { 0 }.
Hence, also this class is not a group, but so far the operators are
commutative: a + b = b + a, etc, but there is no well defined inverse for this
operation.
This leads us to define the operators of A as '+' and '-' in the following
sense:
a + b = c, where a, b and c can be implemented as the C++ std::set<T>, where
T is capable of uniquely identifying equality (ie, if i1 and i2 are two
distinct objects in your inventory, than, expressed as T it holds that i1 ==
i1, and i1 != i2. Ordering can be allowed to be arbitrary (i1 < i2 ==> i1 !=
i2). Then, adding b to a can be implemented by adding the elements of b to a
one by one. Likewise, a - b can be implemented by trying to find and erase all
elements of b one by one from a.
- For the W class the order DOES matter.
If you put on two shirts, the result should be different if you first put on
shirt u and then v, or the other way around: u + v != v + u.
Hence, in this case, on top of the above (for class A) we need to exactly
describe operator< so that the ordering is a well known function of the order
in which these wearables were put on. Baking can then be performed by running
over all elements of a set in order.
Clearly, the above has naturally led us to two operators: + and - to be defined
for the complete outfit vector. While we can easily map these two operators on
the operators of class W, which naturally maps to the operators of class A.
[Note that we do not necessarily have to map them also to the same + and - as
defined above for class S. For example, it is possible to define the vector
operator + and - both as being the operator + for class S.]
Also clearly, we can map the mathematically found operators (+ and -) to the
already existing intuitive phrases in-world as follows:
operator+ : Add to outfit
operator- : Remove from outfit
operator= : Replace outfit (define as: x -= x; x += y) [Note: this is NOT the
same as the '=' that used everywhere else, which indeed just completely
replaces as usual. I will write explicit operator= to mean this operator, and
just '=' everywhere else]
The notion "Incomplete Outfits" now, simply means that we allow the right-hand
terms of each operator to have elements of class S that ARE empty.
As a result we just have to refine the operators of class S, as follows: x + 0
= x and x - 0 = x.
Note this makes operator= semi-non intuitive (note that x - x = x, as per our
definition x - y = x):
If y is empty, then x 'operator=' 0 ==> x -= x ; x += 0 ==> x = x - x ; x = x +
0 ==> x = x ; x = x ==> nul operator.
In other words, assigning a 0 to an element of class S with operator= has no
effect.
I believe that the above definitions, which arise from a mathematical point of
view and not from in-world experience, is sufficient to write a good C++
implementation,
define the right classes and operators for those classes.
Hopefully this analysis was helpful for your cause,
Aleric
- Aleric
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/14/#review59
-----------------------------------------------------------
On 2010-12-13 07:08:28, Vadim ProductEngine wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/14/
> -----------------------------------------------------------
>
> (Updated 2010-12-13 07:08:28)
>
>
> Review request for Viewer.
>
>
> Summary
> -------
>
> Enabled the "Replace Current Outfit" option for incomplete outfits (i.e.
> those that don't contain full set of body parts).
>
>
> This addresses bug STORM-702.
> http://jira.secondlife.com/browse/STORM-702
>
>
> Diffs
> -----
>
> indra/newview/llappearancemgr.cpp 3d2e71443c58
> indra/newview/llinventoryfunctions.cpp 3d2e71443c58
>
> Diff: http://codereview.secondlife.com/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vadim
>
>
_______________________________________________
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
