Sit target config

[kf6kjg]

This PR changes the default legacy sit target offset system to one that seems to me to match SL much more accurately than the previous system.

A new INI option has been added to the sample INI, defaulting to the new mode to help with new grids and personal test grids where SL compatibility is likely to be higher priority than InWorldz compatibility. However, changing it to "Legacy" will restore the offsets used in InWorldz and every other Halcyon-based grid up until now.

I'm requesting that this not just be code reviewed, but actually heavily tested in A-B comparisons between SL and this code in SL-compat mode, as well as between the previous version of Halcyon and this code in Legacy mode.

[kf6kjg]

And yes, I apologize to the code reviewers: I did do some cleanup on the file... I placed that in its own commit, so if you just want to look over the core logic changes, check the first commit.

I could have done a ton more cleanup, but I tried to keep myself to only the most egregious fluff.

[appurist]

"This PR changes the default legacy sit target offset system to one that seems to me to match SL much more accurately than the previous system."
The existing code is 100% compatible with SL when scripted positioning is used. It is also 100% compatible with sit targets except for the <0.1, 0.0, 0.3> OpenSim correction which is applied in very specific and heavily-tested conditions. To counteract that, the simplest and most compatible way would be to alternatively set that to <0.0, 0.0, 0.0>. There is no need to change the logic in any way.

The existing code has undergone months of extensive testing and about a dozen revisions to attempt to get the behavior just right. What specific bug, other than the OpenSim-compatible offset, do all these code changes attempt to resolve?

I cannot support these changes, nor the massive burden of attempting to fully retest all the sit target combinations of sit target plus Avatar-as-a-Prim (AaaP) semantics. This is a disaster waiting to happen, and needless risk. Again, what specific bug do these changes attempt to resolve?

[kf6kjg]

In understand that heavy amounts of testing went into the existing code, and I tried to make sure that if Legacy mode was selected that there was no semantic change.
However I can plainly observe, after some simple A-B comparisons running two sessions of Alchemy side my side on the same machine with one in SL and one in a Halcyon grid that it was more than that offset that was different.

My test setup in each grid, carefully made identical in each grid:

• Two sets of avatar shapes: one large, one small.
• A narrow pointy prim attached to my avatar at Avatar Center.
• A root prim at zero rotation with the root prim having a sit target at <0,0,1> and a zero rotation, with a small pointy prim that lines up, in SL, with the tip of the prim my avatar is wearing. In the Halcyon grid this prim is placed at the same position as SL. This script also plays an animation that packs my avatar into a tidy package to get out of the way.
• Same as the above but with the root prim rotated to an arbitrary 3-axis non-45deg rotation. The given rotation is set the same in both grids.
• Same as the previous but with a new root prim at zero rotation, making the prim with the sit target now a child prim.
• All three of the above again, but with the sit target having been rotated around the Y axis 270deg.

From my observations of SL:

• Standing, changing avatar shape, and sitting again does not change the pointer location.
• The actual center of the avatar appears to be governed by three offset vectors: the offset set in the script via the call to llSitTarget(), a server-side offset that's in the sit-prim rotation, and another server-side offset that's in the sit-target rotation. Experimentally I determined that the latter two offsets are <0,0,0.4> and <0,0,-0.05> respectively. Thus you could predict the actual avatar center location in object-local coordinates via avatarCenter = (sitTargetOffset + <0,0,0.4> + <0,0,-0.05> * sitTargetRotation) * sitPrimLocalRotation
  Interestingly it looks like my old copy of the Sit Target Setter by Lex Neva has the values <0,0,0.186> and <0,0,0.4>.

From my reading the code and observations of unmodified Halcyon and modified Halcyon in Legacy mode:

• Standing, changing avatar shape, and sitting again does change the pointer location: the code incorporates the avatar's HipOffset.
• The actual center of the avatar is governed by two offset vectors and the above HipOffset: the offset set in the script via the call to llSitTarget() and a server-side offset that's in the sit-prim rotation. The latter is <0.1,0,0.3-HipOffset>. Thus you could predict the actual avatar center location in object-local coordinates via avatarCenter = (sitTargetOffset + <0.1,0,0.3 + avatarHipOffset>) * sitPrimLocalRotation Note that HipOffset is typically a negative value.

The problem I'm trying to solve is that various sit target scripts, including my old Lex Neva Sit Target Helper, are tuned for SL sit target math. Grids that are small and taking their scripts from SL, like Vin does, are needing to be redone - and not every scripter has good vector match chops. Thus I'd prefer that Halcyon be SL compatible by default and that those grids or regions on a given grid that want to use the legacy math can do so.

You'll note that in the change that in legacy mode m_sitTargetCorrectionAgentSpaceOffset is a zero vector, so multiplying that by the sitInfo.Rotation and adding to the avSitPos makes no change.

As to testing I'm more than willing to run the tests myself if I can get them. Got an OAR full of such tests? No rush: I'm not going to be merging this PR for a very long time I suspect: it's got a long way to go to prove itself.

[kf6kjg]

I copied my little battery of tests and included the following entries after the avatar is seated:

                // And get into avi-as-a-prim mode.
                llSetLinkPrimitiveParamsFast(llGetNumberOfPrims(), [PRIM_POS_LOCAL, <1,1,1>]);
                llSetLinkPrimitiveParamsFast(llGetNumberOfPrims(), [PRIM_POS_LOCAL, <0,0,1>]);

And tested on SL, unmodified Halcyon, modified Halcyon in SL-compat mode, and modified Halcyon in Legacy mode.

SL, unmodified Halcyon, modified(SL-compat), and modified(Legacy) all resulted in identical avatar position results in the simple rotation cases. However in the complex rotation no edition matched each other. This implies that I did break something in Legacy, that I've not completed SL-compat, and that the unmodified code didn't match SL perfectly even in avi-as-a-prim mode.

Below is an image of three viewer instances: SL on the left, modified(Legacy) Halcyon in the middle, and unmodified Halcyon on the right.

As you can see there's a rotation change getting into the mix somehow on my modified code, but you can also see that the unmodified Halcyon doesn't match SL even in the AaaP case.
(EDIT: Even rewinding back to 0.9.41 I still see that funky rotation. Might be something bugged in BasicPhysics on Linux then. Gonna have to repeat tests running on Windows..)

Interpreting the hovertext

Starting at the top line above the avatar:

• the object-local position of the avatar, as computed from the below information,
• the root prim position in the region,
• the root prim rotation in the region,
• the worn prim's position, and thus the avatar's position, in the region,
• the worn prim's rotation, and thus the avatar's rotation, in the region.

The hover text on the arrow prim the worn prim is supposed to be pointed at is the local position and rotation of the arrow prim.

The hover text on the sit prim is the offset and rotation of the sit target.

[kf6kjg]

I've backed out the code cleanup, and put it in its own branch for a later PR.

[kf6kjg]

I'm seeing that funky rotation even on my Windows box, so I've been investigating when I can.

[kf6kjg]

And I just realized I'm observing it on a region I thought was fine: the same region, the same script, the same prim as the previous test on the right:

I have noticed some odd behavior, both on the live 0.9.40 release and on my locally compiled variants: occasionally my avatar will wind up visually not where it was specified by the sit target to be. If I can find a repro case or if I can prove that it wasn't a script fault I'll open an issue or find a fix.

[kf6kjg]

With the above commits I just got the AvatarSitResponse message coming up the same in SecondLife mode as it does in SecondLife.

For my test setup here was SecondLife's AvatarSitResponse message:

[SitObject]
    ID = 36a7f2cc-8351-4589-ccaa-b274c3f36e80
[SitTransform]
    AutoPilot = 1
    SitPosition = <0.05, 0, 1.4>
    SitRotation = <-0.00903269, -0.956525, -0.134431, 0.258661>
    CameraEyeOffset = <0, 0, 0>
    CameraAtOffset = <0, 0, 0>
    ForceMouselook = 0

Halcyon was giving me the following:

[SitObject]
    ID = 468fa575-cade-436a-9ecd-d51e877c0869
[SitTransform]
    AutoPilot = 0
    SitPosition = <0, 0, 0>
    SitRotation = <0, -0.707107, 0, 0.707107>
    CameraEyeOffset = <0, 0, 0>
    CameraAtOffset = <0, 0, 0>
    ForceMouselook = 0

I also discovered that I'm getting a later message, a ImprovedTerseObjectUpdate, that's making my rotation go wonky if the script contains a call to set PRIM_LOCAL_POS on the seated avatar without specifying a PRIM_LOCAL_ROT.

[RegionData]
    RegionHandle = 109951162803456
    TimeDilation = 65535
[ObjectData]
    Data =| 8C 78 74 9C 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80 3F 00 00 00 00 00 00 00 00 00 00 80 3F 00 80 00 80 00 80 FF 7F FF 7F FF 7F D8 7E 90 05 CA 6E 1B A1 FF 7F FF 7F FF 7F 
    TextureEntry = 

[appurist]

As discussed privately, I will probably never be comfortable with any change to sit target offsets that result in changes for existing content. I introduced a bug in the past that unexpectedly cleared sit targets, and that was my biggest regret in all of InWorldz and Halcyon. However, this PR does not make permanent changes to sit target offsets, only their perceived effect on the avatar, which persists only as long as the object is in a region running these code changes.

Furthermore, I have reviewed the changes and the general approach and technical detail seems sound. I am not in a position to verify the specific results, but I do not see any issues with the code changes, especially with the compatibility break being configurable in the halcyon.ini file on a per-region basis, and appear to be revertable should there be unexpected badness. Of course that INI setting means a piece of furniture with flawless sit target offsets in one region may have a different offset if rezzed in another region. I am not sure I would proceed if I was setting policy, this is a tough one, but I am no longer the gatekeeper for Halcyon or and a code review isn't for setting policy; I don't see any technical errors in these changes, and there are many trade-offs to be considered when deciding policy here. One consideration is the amount of content affected, today, vs potentially in the future. If such a change is to be made, the sooner the better. At any rate, I approve the code changes and leave the decision to merge as-is, or with further tweaks, up to @kf6kjg in this case.