[opensource-dev] Review Request: STORM-1793 1) Treat all mini-map altitudes above 1020 m as the same height 2) Improve z-level accuracy

[Jonathan Yap]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/545/

Review request for Viewer. By Jonathan Yap. Description The SL simulator has recently been fixed so that the CoarseLocationUpdate message properly returns 255 for all altitudes above 1020 meters. The code for the mini-map, in drawing the agent locations for equal, above or below needs to take this into account. It currently does not. The routine that returns who is nearby can be enhanced to scan the character list and use that position data. This gives an accurate z-level value, even when above 1020m. In the case where the relative Z value between you and another avatar is unknown display an X on the mini-map. Testing See test plan in jira Bugs: STORM-1793 Diffs doc/contributions.txt (0010858de5a1) indra/newview/llnetmap.cpp (0010858de5a1) indra/newview/llworld.cpp (0010858de5a1) indra/newview/llworldmapview.h (0010858de5a1) indra/newview/llworldmapview.cpp (0010858de5a1) View Diff

[Zi Ree]

Am Freitag, 27. Januar 2012, 14:40:01 schrieb Jonathan Yap:

> The SL simulator has recently been fixed so that the CoarseLocationUpdate
> message properly returns 255 for all altitudes above 1020 meters.

"Properly" is a very ... unusual term for this kind of behavior. It still
returns a wrong value, just a slightly more wrong one than before.

> Jonathan Yap

Zi
_______________________________________________
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

[Oz Linden (Scott Lawrence)]

On 2012-01-27 13:45 , Zi Ree wrote:
> Am Freitag, 27. Januar 2012, 14:40:01 schrieb Jonathan Yap:
>
>> The SL simulator has recently been fixed so that the CoarseLocationUpdate
>> message properly returns 255 for all altitudes above 1020 meters.
> "Properly" is a very ... unusual term for this kind of behavior. It still
> returns a wrong value, just a slightly more wrong one than before.

'wrong' is not really a relative term...

The new value (255) is at least in the same direction for anyone under
the maximum coarse location altitude, which removes some silly edge
cases in which the reported altitude is rises until is suddenly becomes
zero.

In any event... this is the change that we've decided to make, and
Jonathan appears to have done a good job of making the best of an
admittedly awkward situation. Short of a major change to the coarse
location messages (which would be completely incompatible with older
viewers), I think this is the best that can be done.

[Ricky]

A couple of questions Oz:

1. Would the client have a problem if another word was added to the message giving - duplicating the data, but allowing newer clients to choose to use the new word while older clients use the old byte?
2. Would the client have any problems if the CoarseLocationUpdate message was never sent?  If not then that message could be deprecated, with a removal date set for some time in the future, while a new message that could handle much higher detail information was put in place and all newer clients were switched to it?

I ask because this limitation of the message really puts a pinch in https://jira.secondlife.com/browse/VWR-27577.

Ricky

Cron Stardust

[Jonathan Welch]

> Would the client have a problem if another word was added to the message
> giving - duplicating the data, but allowing newer clients to choose to use
> the new word while older clients use the old byte?

Yes, because the coarseupdate packet holds many positions, it is not
just 1 packet per avatar, but as many as can be packed into the
coarseupdate packet as will fit. So it is not possible to alter this
packets' format in any way. Doing so would break all existing viewers
that expect it to have its current format.

> Would the client have any problems if the CoarseLocationUpdate message was
> never sent?

Yes, with a small exception, you would not know where anyone is anymore.

> date set for some time in the future, while a new message that could handle
> much higher detail information was put in place and all newer clients were
> switched to it?

This has been discussed several times at the server group meetings.
Fixing this just to have a fully correct map display is more trouble
than it is worth; having to run two packet formatters in parallel
forever, having to query the viewer what packet format it wants, etc.
is just not worth such a big effort.

[Kadah]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/27/2012 3:37 PM, Jonathan Welch wrote:
>> Would the client have a problem if another word was added to the
>> message giving - duplicating the data, but allowing newer clients
>> to choose to use the new word while older clients use the old
>> byte?
>
> Yes, because the coarseupdate packet holds many positions, it is
> not just 1 packet per avatar, but as many as can be packed into
> the coarseupdate packet as will fit. So it is not possible to
> alter this packets' format in any way. Doing so would break all
> existing viewers that expect it to have its current format.
>
>> Would the client have any problems if the CoarseLocationUpdate
>> message was never sent?
>
> Yes, with a small exception, you would not know where anyone is
> anymore.
>
>> date set for some time in the future, while a new message that
>> could handle much higher detail information was put in place and
>> all newer clients were switched to it?
>
> This has been discussed several times at the server group
> meetings. Fixing this just to have a fully correct map display is
> more trouble than it is worth; having to run two packet formatters
> in parallel forever, having to query the viewer what packet format
> it wants, etc. is just not worth such a big effort.

I just wanted to add that the message containing all agents is only
packed once per update, then sent in bulk to all agents.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJPIzbGAAoJEIdLfPRu7qE2/lIIALVQq2mlITepS90gzriRFRPh
BnFrnddJk7aD9J+SUtAw11FFqDqF+wGDX0l0Iu1oJqaPPtgjLweTXg0xNLzTOVIz
/fdh1Nr711EdIoMZZFPWhpH/SP2lb7DwhZhmREONs2aAdlX1AKahaMclkbhJOFvY
SGF1BAK5RrmewQFvgr96uiHJQaXwjo44DFpRWmeiCrCtCapPNu5U7Fz4iCGS7LxG
kL6pSLBaDM7VcqkL3vHHALHeXgKLvLsEDqS36hwvTyNlWuqci7A1OZC6iKD2sNDz
03l7y8HnK9OWRcWhUxWoUbQM/ig33T1Ka0ZcuYESE1T9mB1ETlgKiyy3v9icmgo=
=3c8S
-----END PGP SIGNATURE-----

[Ricky]

It just means that having a proper clientside radar system is now truly stuck.

And I did mean a finite time period for the deprecation: I agree that such things are unmaintainable in the long term.  However the current state of affairs isn't viable for long either, so a plan has to be readied.

Kadah's note also shows that it is something that is not able to change per-viewer, so the only viable route is the deprecation model: for a period of time, say one year, the old message AND the new message are sent to all clients.  The older clients should discard any message they don't recognize, in this case the new message, while the newer clients will discard the older message for the same reason.  After the deprecation period is over the old message code is purged leaving only the new message and anyone who is running an un-updated client simply no long can tell altitude differences in the minimap.

As to the format, one byte is not great, but now I at least understand why it was chosen: maximum data density.  Currently the message uses absolute coordinates interpreted linearly across the space in the range [0, 255] * 4 meters.  This worked when the region was only 1024m high.  Here is another idea, trying to keep in what I think was the original spirit of the message: switch to using dynamic scale.  Use the existing packet format, therefore keeping the bulk of the code the same, but have the client and the server agree that the scale value, currently set at 4 meters per message unit, is to be computed from the size of the region.  This would mean that old-school 1024-meter-high regions would have the calculation for the scalar as so: 1024 / 255 = 4, resulting in the current value that worked so well for those regions. (Yes, I know such regions no longer exist.) Current regions would have the scalar calculated as 4096 / 255 = 16.  Future regions with even higher ceilings would just continue that idea, resulting in the message's number being more like a percentage.  Just make sure that this math is done for X and Y as well - not just Z.  Otherwise we are right back here again if in the future estates can choose to have a different region size than other estates...  (And yes it could be done.  A LOT of work, but it could be done.)

Ricky

Cron Stardust

PS: I really want to be able to get rid of my lag-creating Sensor-based HUD for a pure client-side system.  Hence my active commentary.

[Jonathan Welch]

> PS: I really want to be able to get rid of my lag-creating Sensor-based HUD
> for a pure client-side system. Hence my active commentary.

The server team is not going to be making changes to the current
system; they have said the cost of doing that, the additional load on
the servers, etc. is not worth having more accurate z values. Much
better for them to put their efforts on more important changes, etc.

Now, given all that, the LL viewer is about to catch up to what the
TPVs have been been doing for some time. It will use Z data for
avatars near you, so if you are at 2,000m and someone is not too far
away then your mini-map will show a proper height icon. The other
part of that change will be to show an "X" if the relative height of
you and someone else is not known (no more down pointing chevron for
group of people standing around together at 2,000m).

[Arrehn Oberlander]

On 1/27/12, Jonathan Welch <jhw...@gmail.com> wrote:

> This has been discussed several times at the server group meetings.
> Fixing this just to have a fully correct map display is more trouble
> than it is worth; having to run two packet formatters in parallel
> forever, having to query the viewer what packet format it wants, etc.
> is just not worth such a big effort.

It's 2012. If this is seriously the official reason why SL's premier
client can't support more than 8 bits of z-height data, no one's
buying it, least of all professional software developers. I could have
said that same thing for 2002 as well.

Wake up people.. More than 1/2 of the grid is already working around
this architectural deficiency via all manner of LSL-based hacks. What
do you think the cost of that is compared to making it right? In one
word: apalling and embarassing. As it should be.

[Argent Stonecutter]

How about reducing the vertical resolution of the packet by 4?

[Kadah]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/27/2012 6:21 PM, Argent Stonecutter wrote:
> How about reducing the vertical resolution of the packet by 4?

This was also brought up several times during the various discussions.
It would break old/existing clients more than the current change, and
the vertical resolution of 12m is a very significant height
difference, agents could be on 2 or 3 different floors of a building
and all report as being at the same level.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJPI10SAAoJEIdLfPRu7qE2St8IAJOVYz59LYlbaD+w4jRNonKC
qw9oP30TkNU25SUVVonm+9MF8X1jAv0a2m+laXDFReyBnVr8r55xXs7R7VEotc+k
+0XThCYUxW61ildjekaUXz5fx1OefwX3Ds1kq0EOEmQ+XFq8Udkues18LNd77fTc
rKRYfXr/2y64Y1x6Wj4c1DVbz559FeVPMzzDss+Va8wDFwrliPk06r/SOo/PrtWh
BIhFik0icNlub8fHGZOIL/aLKTO14OSybazzuKW56pU8b56hHZV2kr3X4hXMRZxM
ltRgNRjsFBnOGXPKMYS9x2r0ChNxOeTdoAFhRqdOfZA1INLi1wdfDDyD7aihPYU=
=egRA
-----END PGP SIGNATURE-----

[Armin Weatherwax]

Am Saturday 28 January 2012 00:37:44 schrieb Jonathan Welch:
> Yes, because the coarseupdate packet holds many positions, it is not
> just 1 packet per avatar, but as many as can be packed into the
> coarseupdate packet as will fit.  So it is not possible to alter this
> packets' format in any way.  Doing so would break all existing viewers
> that expect it to have its current format.

Old viewers have all sorts of issues if their code isn't maintained anymore,
most of them are way more important.

Imagine the Z- byte transformation was made non-linear: that would still give
better results than we have right now, be a small change to new viewers,
have low impact on old viewers, and generate no extra network load.
Example: (1*1 + 2*2 + 4*4 + 8*8 + 16*16 + 32*32 + 64*64 + 128*128)m = 21845m
range.

Armin

[Nicky D.]

Or you could as well make a new message, then alternate it with the current one.
Eg. instead of sending CoarseLocation update every x (lets say 5 )
seconds,you send
0 - CoarseLocation
5 - CoarseLocationNew
10 - CoarseLocation
15 - CoarseLocationNew

And so on.

- Old clients would lose update precision. But not be broken.

- New clients would use both messages and get X/Y updates at the same
interval as today.
For Z they could interpolate if the old message send an 'out of
range'-Z value. Or just live with
the lower Z update frequency. Both would still be vastly better than
what we have now.

- The amount of message the simulator has to send would not increase.

Phase out the old message by reducing its frequency after a few month, then stop
sending it at all. Whoever did not update their code by that time
could be considered
badly outdated.

[Jonathan Yap]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/545/

Review request for Viewer. By Jonathan Yap.

Updated Feb. 3, 2012, 6:05 a.m. Changes Fix bad assumption in getAvatars

Description The SL simulator has recently been fixed so that the CoarseLocationUpdate message properly returns 255 for all altitudes above 1020 meters. The code for the mini-map, in drawing the agent locations for equal, above or below needs to take this into account. It currently does not. The routine that returns who is nearby can be enhanced to scan the character list and use that position data. This gives an accurate z-level value, even when above 1020m. In the case where the relative Z value between you and another avatar is unknown display an X on the mini-map. Testing See test plan in jira Bugs: STORM-1793

Diffs (updated)

[Jonathan Yap]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/545/

Review request for Viewer. By Jonathan Yap.

Updated Feb. 3, 2012, 8:51 a.m. Changes Fixed another logic error in getAvatars. This code could be improved by changing one call into this method, so avatar_ids always has a matching positions parameter (which, erroneously, I thought it did).

[Oz Linden]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/545/

Ship it!

indra/newview/llworld.cpp (Diff revision 3)
void send_agent_resume()
		1193	if( !pVOAvatar->isDead() &&
		1194	!pVOAvatar->isSelf() &&
		1195	!uuid.isNull() &&
		1196	dist_vec_squared(pos_global, relative_to) <= radius_squared)

When wrapping conditions across lines, I find it more clear to put the operators on the left:

if (   condition1
    && condition2
...

- Oz

On February 3rd, 2012, 8:51 a.m., Jonathan Yap wrote:

Review request for Viewer. By Jonathan Yap. Updated Feb. 3, 2012, 8:51 a.m.

Description

The SL simulator has recently been fixed so that the CoarseLocationUpdate message properly returns 255 for all altitudes above 1020 meters. The code for the mini-map, in drawing the agent locations for equal, above or below needs to take this into account. It currently does not. The routine that returns who is nearby can be enhanced to scan the character list and use that position data. This gives an accurate z-level value, even when above 1020m. In the case where the relative Z value between you and another avatar is unknown display an X on the mini-map. Testing See test plan in jira Bugs: STORM-1793 Diffs

[Jonathan Yap]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/545/

Review request for Viewer. By Jonathan Yap.

Updated Feb. 7, 2012, 6:28 a.m. Changes Fix syntax error. Forgot to compile after making the last change. Dumb mistake.

[Oz Linden]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/545/

Ship it!

Ship It!

- Oz

On February 7th, 2012, 6:28 a.m., Jonathan Yap wrote:

Review request for Viewer. By Jonathan Yap. Updated Feb. 7, 2012, 6:28 a.m.

Description

The SL simulator has recently been fixed so that the CoarseLocationUpdate message properly returns 255 for all altitudes above 1020 meters. The code for the mini-map, in drawing the agent locations for equal, above or below needs to take this into account. It currently does not. The routine that returns who is nearby can be enhanced to scan the character list and use that position data. This gives an accurate z-level value, even when above 1020m. In the case where the relative Z value between you and another avatar is unknown display an X on the mini-map. Testing See test plan in jira Bugs: STORM-1793 Diffs