PR#950 breaks ALL existing shapes

[FelixWolf]

Environment

N/A

Description

The change made to avatar_lad.xml will break existing shapes, and is incomplete code wise. Ref: https://github.com/secondlife/viewer/pull/950/commits/028fc448322b2388759703aa466c09c7c0d585ec#diff-6671a74c3f95a8f854245aa102deb48aa2cbcacd695a73df202a2fca66cd6abaL6920

The way avatars work is that once the gender slider is greater than 0.5f, code sees the avatar as male, otherwise female. Various parameters use this to drive specific blend shapes and morphs depending on the avatar shape, simply removing all instances of sex="male" or sex="female" without understanding what they do will cause problems.

Specifically the following issues are present:

1. This breaks existing shapes because the parameters for boobs are disabled for males, but aren't set to zero, as a result, male shapes will unintentionally have boobs on viewers using this change. See LLPolySkeletalDistortion::apply.
2. There are now two Muscular_Torso params (Due to the fact there was one for each sex). Originally these were separated for female and male blend shape.
   
3. Setting the gender slider to 0 will not enable the boobs sliders, as the trigger for that is tied to onCommitSexChange from the AvatarSex control.
4. In addition to above, this may not update various code properly.
5. There is unused code tied to elements that are no longer accessible.

We were testing enabling this in Alchemy the other day(See https://git.alchemyviewer.org/FelixWolf/alchemy-viewer/-/commit/e0c37a4a017cc869670ea53554a7856789138d31) and there were a few hiccups. Noteably: My shape became corrupt. (I managed to rescue it by reading logs and dumping the asset manually and re-creating it) There is a chance this was coincidence, but this needs to be ruled out before full merge into the viewer. In specific, the shape was no longer in the system because it apparently didn't pass validation, but the inventory asset pointer was updated anyway.

Reproduction steps

N/A

[vldevel]

Yes, years ago, I did test this (old) tweak too, for possible inclusion in the Cool VL Viewer and got the same bad issues, which caused me to reject it entirely.

Please, revert and keep at bay !

[marchcat]

Thanks! Reverting..

[bennettgoble]

The original PR mentioned the torso muscle parameters issue needed a resolution:

A decision will have to be made as to what the formerly Male and Female 'Torso Muscle' sliders should be named as currently you will have both with identical names.

In addition, some other items, such as the creation of new default shapes, were identified. It's safe to say that some more work needed to be done before calling this feature complete: the original contribution said as much :)

The appropriate response here is probably to first reach out to the original contributor rather than immediately reverting changes. At least that is the response I'm used to if there are bugs (or just perceived bugs) in my changes to other open source projects. At a minimum let's chat with folks in the coming week to explore what would be necessary to add a useful new enhancement.

I think we need to encourage more of a working relationship than a knee-jerk type situation with incoming PRs and issues: work with OSS contributors like they are maintainers. There also needs to be more thoughtful consideration of possible remaining work while evaluating PRs: is this feature complete, does it need assistance, what work remains? The original PR identified the fact that some work is necessary, and yet it was merged rather quickly.

---

Regarding some specifics outlined above:

This breaks existing shapes because the parameters for boobs are disabled for males, but aren't set to zero

The male body shape has values for these parameters by default, for example, the default male shape has as Breast Size value of 0.5:

<param id="105" name="breast size" display="Breast Size" value="0.500" u8="127" type="param_driver" wearable="shape" group="0"/>

You are right, @FelixWolf, that the radio button behavior tied to onCommitSexChange will need to be updated, as well as some other small assumptions, but so far I don't see anything really fundamental blocking us from moving forward as long as the nuances are addressed?

---

On another note: I would suggest using less bombastic and superlative issue titles: "PR#950 breaks ALL existing shapes" --this is not true, or is at best an exaggeration: existing shapes are not broken by this change.

[Mezzith]

There's no problem with this just more options available. It will confuse automated furniture that relies on a single value to determine gender/sex/whatever word. But there's a swap button. And a lot of furniture assigns male or female seat first on sit. And there's a swap button. I've had shapes corrupt ages ago out of nowhere becuase of asset/connection issues never cause of client. Was very bad for a week or 2 then haven't seen it in over a year. SL fun c:

[marchcat]

Please excuse me for the late response on this.

1. I usually merge the contributions very fast after just a quick code sanity check, so we can test and see how they work;
2. Target branches for these merges are somewhere close to the end of the queue (like Maint B for this one, while we still have Maint W in development) - this gives us enough wiggle room for further changes;
3. Issue named "it BREAKS ALL shapes" obviously sounds dangerous enough to immediately revert the change and continue development separately until it's ready for another round; however, I see that's not the case now. Thanks for the clarifications.
4. I haven't reverted it yet, just created a PR #958 which is still open; let's keep it open for now as a fallback option.