|
|
|
[
Permlink
| « Hide
]
Rob Linden added a comment - 03/Sep/09 10:18 AM
Hi Pixel, this is great! The biggest worry with this code is make sure that this has a test plan, particularly with respect to non-OGP usage (current production grid). Test plan should include elements like region crossing, teleport, login, and avatar rezzing.
I added a Test Plan to the wiki, focusing, as you suggested, Rob, on non-OGP testing.
https://wiki.secondlife.com/wiki/User:Pixel_Gausman/Interop_Viewer#Test_Plan Patch review: code seems OK and builds (Windows) though I have not tested it yet. There was numerous code convention issues so I cleaned up the whole thing and posted a new patch here (most had to do with proper use of LL_ENDL, use of brackets, comments typos). I modified a little the logic in only one file (actually, the logic is the same, I just condensed the writing).
I reviewed the OGP code itself pairing with Infinity so I think this is good. Last part of my review then will be to do a live test on vaak. Hi Merov,
Thanks for the patch review. I reviewed your new patch, including the logic 'slimming' change in llstartup.cpp. The changes look fine. Apologies for the LL_ENDL, that fell off my radar. Proposal from the OS meeting was to expose the new functionality first under the Advanced menu since, as Pixel says "that's where new things typically get their feet wet".
fwiw.. there's a list of regions responding to OGP protocol at https://wiki.secondlife.com/wiki/Open_Grid_Public_Beta/Public_Regions
 . obviously, we (linden) can't claim responsibility for any of them but the ones we own. This includes the patch Merov made plus it moves the "Teleport Region..." menu item from World to the Advanced menu in a new submenu called Interop.
I made these changes: added code to init_client_menu() to add the Interop submenu to Advanced menu It actually ended up being slightly cleaner, I only add the Interop submenu to the Advanced menu if the viewer is running in OGP mode, instead of it always being there, and additionally removed the two places I was enabling/disabling the menu item. Hi Pixel,
Several problems:
Looks like there's a little bit more work to do on that one. Details, details... Thanks for the heavy lifting on that one though. It's pretty close now and I hope we'll nail it and commit it next week. The llstartuppatch (didn't attach last time around...)
hmm.. i've been having some problems with some of these patches on the mac.
specifically, i tried applying pixel's fwiw, i'm able to build rev 2757 with no problems, and i've double checked i have the right versions of OS and XCode, so i don't think my problem is related to build environment. specifically i get errors in llstartup.cpp on lines that assign an element of a mResult into a std::string. my guess is that gcc on the mac is mildly more strict than msvc as it reports that we're attempting ambiguous operator overloading. to help the compiler out, i added a ".asString()" to the end of each of the offending lines and the compiler seems to be happy. so what i did: 1. check out rev 2757, put the libraries in the right place, grab fmod, etc. etc. as documented on the wiki. i have a script that does this automagically at https://wiki.secondlife.com/wiki/User:Arrogant_Cyberstar/Compiling_Snowglobe_on_MacOSX 2. apply 3. cd to indra/newview, revert llstartup.cpp with the command: svn revert llstartup.cpp 4. apply 5. continue building. i also tried applying merov's llstartup patch, but after that it claimed to not know about the --ogp option, which seemed "bad." so... i haven't gotten it running on the mac with merov's llstartup fix yet. The attached patch combines Merov's and Arrogants patches with mine, gets rid of the extra menu separator.
Hi Arrogant, Hi Merov,
Hi all,
Great to see lots of activity on that one  Thanks for taking in my changes Pixel. Unfortunately, I still wasn't able to see the new "Interop" menu in my test, even when loading on vaak using ogp. I did the following modifs:
Attached is a patch ( I still need to check Mac though Just got pinged by Pixel on Twitter (how cool!). I'm really puzzled she could see the 'Interop' menu so I'm thinking I may have screwed up some with all those patches... Rebuilding her stuff from scratch and see what happens (still, my shortcut change still stands...
) OK, I'm taking my change back: the Interop sub-menu does show in "Advanced". My bad...
The short cut should be changed though to avoid collision with the Admin menu. Last: I can compile on Mac but haven't been able to run with --ogp from the command line: I get an "unknown option ogp". Apparently something missing on Mac for the command line parsing to work. Will explore further... OK, works on Mac. My patch is wrong wrt cmd_line.xml (I don't know why...) but reapplying Pixel's patch
Waiting for Pixel's comment before committing it in Snowglobe and resolving that PJIRA record. Thanks again to Arrogant and Pixel for the interactive discussion. OK. Hit the giant red button and commit.
Thanks for such prompt attention on this, Merov. Also, I've updated https://wiki.secondlife.com/wiki/User:Pixel_Gausman/Interop_Viewer#Test_Plan
to reflect the new shortcut, and also clarified the size of the Start Location combo box for OGP vs non-OGP usage. Hi Pixel, a couple of pieces of housekeeping we noticed:
1. Several spots in the code have this: //Copyright International Business Machines Corporation 2008-9
The placement of this under the license makes it look like the IBM's contribution isn't under the same license as the rest of the code, which I'm assuming isn't the intent. IBM still holds joint copyright (since the contrib agreement is joint copyright), so we don't need to nuke the notice. Unfortunately, we can't really put the notice inside the $LicenseInfo$ block because it'll get nuked the next time we run the date replacement script over the top of everything. So, my suggestion is that we just add the following on all of the files you submitted: //Contributed to Linden Research, Inc. under the Second Life Viewer Contribution //Agreement and licensed as above. Mind if we do this before committing? 2. You didn't credit yourself in doc/contributions.txt. We'll add you in there. Anyone else we need to credit? Hi Robla,
The clarification to the copyright statement sounds appropriate. IBM did not intend to contribute this code under a different license than the standard SL Viewer Contribution Agreement. Leyla (LL employee) did some of the initial login coding, but all the rest of it you can blame on me. Revision 2771
by merov.linden on 2009-09-17 15:50:24 -0500 (Thu, 17 Sep 2009)
Files affected: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
New Feature
Resolved
Normal
OGP Interop login/teleport 
