Re: [opensource-dev] Review Request: OPEN-172 Combined changesets for Linux gcc 4.7, 2 build of viewer development
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/618/#review1304 --- Ship it! Glad to see that finally someone removes those 'namespace boost' around intrusive functions. I couldn't get it explained to Oz... - Aleric Inglewood On April 13, 2013, 3:21 p.m., Nicky Perian wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/618/ --- (Updated April 13, 2013, 3:21 p.m.) Review request for Viewer. Description --- OPEN-172 Combined changesets for Linux gcc 4.7,2 build of viewer development This addresses bug OPEN-172. http://jira.secondlife.com/browse/OPEN-172 Diffs - indra/cmake/00-Common.cmake c085df84186a indra/cmake/Copy3rdPartyLibs.cmake c085df84186a indra/cmake/LLAddBuildTest.cmake c085df84186a indra/linux_updater/linux_updater.cpp c085df84186a indra/llappearance/lltexturemanagerbridge.h c085df84186a indra/llappearance/llwearabletype.h c085df84186a indra/llcommon/lldarray.h c085df84186a indra/llcommon/lleventcoro.h c085df84186a indra/llcommon/llinitparam.h c085df84186a indra/llcommon/llrefcount.h c085df84186a indra/llcorehttp/CMakeLists.txt c085df84186a indra/llmath/lloctree.h c085df84186a indra/llmessage/CMakeLists.txt c085df84186a indra/llmessage/lliopipe.h c085df84186a indra/llmessage/llregionpresenceverifier.cpp c085df84186a indra/llui/llview.cpp c085df84186a indra/media_plugins/gstreamer010/media_plugin_gstreamer010.cpp c085df84186a indra/newview/llcommunicationchannel.cpp c085df84186a indra/newview/llfloaterpathfindingobjects.cpp c085df84186a indra/newview/llsurfacepatch.cpp c085df84186a indra/newview/llworld.cpp c085df84186a indra/test/CMakeLists.txt c085df84186a indra/test/test.cpp c085df84186a Diff: http://codereview.secondlife.com/r/618/diff/ Testing --- Built on debian wheezy virtual box virtual machine. Logged on secondlife. Checked logs and found no spamming. Do not have a native machine to test with. The change to llviewertexture.cpp around line 419 needs a close look. Thanks, Nicky Perian ___ 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
Re: [opensource-dev] Review Request: BUG-1709: Tiny prims do not rescale properly at very high viewer framerates
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/617/#review1301 --- I don't understand this fix at ALL. It doesn't seem to make any sense :/... What you need is to set the final scale once the distance between destination and current become LESS than the minimum interpolation distance, because then interpolation will never update it anymore. This fix just makes condition true all the time for your test case at your frame rate, but will still fail to reach the final scale when you increase the frame rate (or when the destination scale is simply so close to the start scale that this condition is never true). - Aleric Inglewood On Feb. 17, 2013, 7:02 p.m., MartinRJ Fayray wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/617/ --- (Updated Feb. 17, 2013, 7:02 p.m.) Review request for Viewer. Description --- At high framerates tiny prims get stuck upon interpolation when they are resized via script. I fixed that by comparing the original scale versus the new target scale (instead of comparing the original scale versus the new interpolated target scale), in lldrawable.cpp updateXform to decide whether a scale change requires an immediate rebuild or not. This addresses bug BUG-1709. https://jira.secondlife.com/browse/BUG-1709 Diffs - indra/newview/lldrawable.cpp fbbee98b7512 Diff: http://codereview.secondlife.com/r/617/diff/ Testing --- See test plan in Jira: https://jira.secondlife.com/browse/BUG-1709 Repository: https://bitbucket.org/MartinRJ/bug-1709 Thanks, MartinRJ Fayray ___ 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
Re: [opensource-dev] Review Request: patch potential memory leak in llgl.h
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/603/#review1265 --- There is semi-colon where it doesn't belong. Otherwise ok (Singularity has this patch since a long time). - Aleric Inglewood On Sept. 19, 2012, 8:26 a.m., Gistya Eusebio wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/603/ --- (Updated Sept. 19, 2012, 8:26 a.m.) Review request for Viewer. Description --- In llvertexbuffer.cpp we call: delete mFence; mFence is an instance of class LLGLSyncFence which is a sub-class of LLGLFence, which is defined in llgl.h. However, class LLGLFence should have a virtual destructor because it's the base class. This patch fixes that potential memory leak, adding a virtual destructor to class LLGLFence. The virtual destructor ensures that the destructor for the derived class also gets called when we call delete mfence;. To quote Björn Pollex, If you have a class that is supposed to be usable polymorphically, it should also be deletable polymorphically. (Unless I'm missing something...!) NOTE: I notice that related code is commented in methods void LLVertexBuffer::placeFence() const and void LLVertexBuffer::waitFence() const -- maybe we commented it out because this memory leak was unresolved? Perhaps it can be uncommented now? I haven't tried yet. There was no note as to why the code is commented out. Diffs - indra/llrender/llgl.h UNKNOWN Diff: http://codereview.secondlife.com/r/603/diff/ Testing --- I did compile this with OS X 10.8 as a build target successfully. I made other changes too, so while my FPS seems improved, it could be from any number of issues. I did notice that any llCharacters that are moving around don't get rendered properly by my build, but I don't know if it's because of this code revision or something else. I need to do further testing on that. Thanks, Gistya Eusebio ___ 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
Re: [opensource-dev] Review Request: OPEN-67: make LLDirIterator implementation compatible to boost::filesystem v3 (as found in Boost 1.44 and newer)
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/313/#review793 --- indra/llvfs/lldiriterator.cpp http://codereview.secondlife.com/r/313/#comment792 What is your reasoning to use native() here and not string()? - Aleric On May 25, 2011, 1:25 p.m., Boroondas Gupte wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/313/ --- (Updated May 25, 2011, 1:25 p.m.) Review request for Viewer. Summary --- Context: We are currently using Boost 1.45, which already comes with the new Boost Filesystem Library API (Version 3) but still defaults to the old one (Version 2). From Boost 1.46 on, V3 will be the default and Boost 1.47 will be the last one to come with V2. The Boost Filesystem Library documentation recommends Existing code should be moved to Version 3 as soon as convenient. New code should be written for Version 3. Version 2 is deprecated, and will not be included in Boost releases 1.48 and later. This change overrides the default, so that the V3 API is used, and makes the necessary code changes. (So we can stick to Boost 1.45 and upgrade whenever we feel like it.) Note: I only changed stuff that the compiler complained about. If the new API also changes semantic of still-compiling library usage, more changes might be necessary. This addresses bug OPEN-67. http://jira.secondlife.com/browse/OPEN-67 Diffs - doc/contributions.txt 959f9340da92 indra/llvfs/lldiriterator.cpp 959f9340da92 Diff: http://codereview.secondlife.com/r/313/diff Testing --- * Compiled Viewer (standalone) with Boost 1.45 * Started Viewer * Logged in * Compiled Viewer (standalone) with Boost 1.46 * Started Viewer * Logged in Not tested: * non-standalone Thanks, Boroondas ___ 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
Re: [opensource-dev] Review Request: OPEN-67: make LLDirIterator implementation compatible to boost::filesystem v3 (as found in Boost 1.44 and newer)
On June 21, 2011, 6:56 a.m., Aleric Inglewood wrote: indra/llvfs/lldiriterator.cpp, line 123 http://codereview.secondlife.com/r/313/diff/1/?file=2823#file2823line123 What is your reasoning to use native() here and not string()? Boroondas Gupte wrote: I tried to stick to a 1:1 replacement. The old boost::filesystem::path::filename() has return type string_type, and boost::filesystem::path::native(), too, so that seemed like the natural replacement to me. As we save the result in a std::string, my reasoning might not make too much sense, though. No that makes no sense at all, since boost::filesystem::path::native::string() also returns a std::string. So, still the same question: why mIter-path().filename().native() and not mIter-path().filename().string()? - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/313/#review793 --- On May 25, 2011, 1:25 p.m., Boroondas Gupte wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/313/ --- (Updated May 25, 2011, 1:25 p.m.) Review request for Viewer. Summary --- Context: We are currently using Boost 1.45, which already comes with the new Boost Filesystem Library API (Version 3) but still defaults to the old one (Version 2). From Boost 1.46 on, V3 will be the default and Boost 1.47 will be the last one to come with V2. The Boost Filesystem Library documentation recommends Existing code should be moved to Version 3 as soon as convenient. New code should be written for Version 3. Version 2 is deprecated, and will not be included in Boost releases 1.48 and later. This change overrides the default, so that the V3 API is used, and makes the necessary code changes. (So we can stick to Boost 1.45 and upgrade whenever we feel like it.) Note: I only changed stuff that the compiler complained about. If the new API also changes semantic of still-compiling library usage, more changes might be necessary. This addresses bug OPEN-67. http://jira.secondlife.com/browse/OPEN-67 Diffs - doc/contributions.txt 959f9340da92 indra/llvfs/lldiriterator.cpp 959f9340da92 Diff: http://codereview.secondlife.com/r/313/diff Testing --- * Compiled Viewer (standalone) with Boost 1.45 * Started Viewer * Logged in * Compiled Viewer (standalone) with Boost 1.46 * Started Viewer * Logged in Not tested: * non-standalone Thanks, Boroondas ___ 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
Re: [opensource-dev] Review Request: OPEN-67: make LLDirIterator implementation compatible to boost::filesystem v3 (as found in Boost 1.44 and newer)
On June 21, 2011, 6:56 a.m., Aleric Inglewood wrote: indra/llvfs/lldiriterator.cpp, line 123 http://codereview.secondlife.com/r/313/diff/1/?file=2823#file2823line123 What is your reasoning to use native() here and not string()? Boroondas Gupte wrote: I tried to stick to a 1:1 replacement. The old boost::filesystem::path::filename() has return type string_type, and boost::filesystem::path::native(), too, so that seemed like the natural replacement to me. As we save the result in a std::string, my reasoning might not make too much sense, though. Aleric Inglewood wrote: No that makes no sense at all, since boost::filesystem::path::native::string() also returns a std::string. So, still the same question: why mIter-path().filename().native() and not mIter-path().filename().string()? I meant boost::filesystem::path::string() - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/313/#review793 --- On May 25, 2011, 1:25 p.m., Boroondas Gupte wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/313/ --- (Updated May 25, 2011, 1:25 p.m.) Review request for Viewer. Summary --- Context: We are currently using Boost 1.45, which already comes with the new Boost Filesystem Library API (Version 3) but still defaults to the old one (Version 2). From Boost 1.46 on, V3 will be the default and Boost 1.47 will be the last one to come with V2. The Boost Filesystem Library documentation recommends Existing code should be moved to Version 3 as soon as convenient. New code should be written for Version 3. Version 2 is deprecated, and will not be included in Boost releases 1.48 and later. This change overrides the default, so that the V3 API is used, and makes the necessary code changes. (So we can stick to Boost 1.45 and upgrade whenever we feel like it.) Note: I only changed stuff that the compiler complained about. If the new API also changes semantic of still-compiling library usage, more changes might be necessary. This addresses bug OPEN-67. http://jira.secondlife.com/browse/OPEN-67 Diffs - doc/contributions.txt 959f9340da92 indra/llvfs/lldiriterator.cpp 959f9340da92 Diff: http://codereview.secondlife.com/r/313/diff Testing --- * Compiled Viewer (standalone) with Boost 1.45 * Started Viewer * Logged in * Compiled Viewer (standalone) with Boost 1.46 * Started Viewer * Logged in Not tested: * non-standalone Thanks, Boroondas ___ 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
Re: [opensource-dev] Review Request: VWR-25965 LLDirIterator fixes in the viewer cache migration and VFS fallback.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/324/#review736 --- Ship it! - Aleric On June 7, 2011, 11:42 a.m., Log Linden wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/324/ --- (Updated June 7, 2011, 11:42 a.m.) Review request for Viewer. Summary --- This removes additional leading delimiters in llappviewer needed due to the STORM-477 changes. This change should fix the fallback used when the viewer cannot find a VFS with the salt it expects to find and instead looks for any VFS data file in the cache directory to use. It also fixes cache directory migration code that runs on Mac and Windows every time the viewer is updated. This addresses bug VWR-25964. http://jira.secondlife.com/browse/VWR-25964 Diffs - indra/newview/llappviewer.cpp 1d4d568986e3 Diff: http://codereview.secondlife.com/r/324/diff Testing --- Test Plan: https://jira.secondlife.com/browse/VWR-25965?focusedCommentId=264386 I ran Case 1 on Linux and Case 2 on Mac and Windows. Everything worked as expected. Thanks, Log ___ 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
Re: [opensource-dev] Review Request: VWR-25862 Fix viewer caches not being cleared.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/315/#review724 --- indra/newview/llvocache.cpp http://codereview.secondlife.com/r/315/#comment691 This patch removes (everywhere, not just here) the delimiter in front of 'mask' passed to deleteFilesInDir. deleteFilesInDir does nothing with mask but pass it to LLDirIterator. Apparently, the introduction of LLDirIterator (new in Viewer 2) requires mask to NOT start with a delimiter. However, you fail to correct this consistently; for example, in LLAppViewer::migrateCacheDirectory we have the following code: std::string mask = delimiter + *.*; LLDirIterator iter(old_cache_dir, mask); Can you explain that works (does it?) and the code that you changed didn't work? - Aleric On May 27, 2011, 1:04 p.m., Log Linden wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/315/ --- (Updated May 27, 2011, 1:04 p.m.) Review request for Viewer. Summary --- This is a patch to fix the issues with clearing viewer caches documented in VWR-25862. I removed the leading delimiter in the place where each of the three caches are cleared. I also removed the mac specific deleteFilesInDir() method, which should have been removed for STORM-477. I removed unneeded calls to getDirDelimiter() in llvocache.cpp. This addresses bugs vwr-25681 and vwr-25862. http://jira.secondlife.com/browse/vwr-25681 http://jira.secondlife.com/browse/vwr-25862 Diffs - indra/llvfs/lldir_mac.h UNKNOWN indra/llvfs/lldir_mac.cpp UNKNOWN indra/newview/llappviewer.cpp UNKNOWN indra/newview/lltexturecache.cpp UNKNOWN indra/newview/llvocache.cpp UNKNOWN Diff: http://codereview.secondlife.com/r/315/diff Testing --- Test plan: https://jira.secondlife.com/browse/VWR-25862?#comment-262800 I built and tested on Mac, Windows and Linux and saw the correct behavior when clicking the clear history button and the move cache button. Thanks, Log ___ 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
[opensource-dev] Outstanding patches since Januari...
This is just a reminder that there are still six patches of me waiting on the reviewboard: https://codereview.secondlife.com/r/88/ https://codereview.secondlife.com/r/92/ https://codereview.secondlife.com/r/80/ https://codereview.secondlife.com/r/95/ https://codereview.secondlife.com/r/81/ https://codereview.secondlife.com/r/91/ [This one also, but that one is currently being processed: https://codereview.secondlife.com/r/93/ Finally there are, https://codereview.secondlife.com/r/167/ https://codereview.secondlife.com/r/99/ ] Those six, together with a bunch of others that were processed in Januari were already done in December last year - but Oz told me not to post them because he wouldn't have 'reviewers' ready, so I waited three weeks. After being posted, seven more weeks have passed-- and all that time it seems to make no sense to do anything else: - Firstly, there are still patches out standing, what's the use to add more? It makes no sense to submit patches faster than they are processed. - Secondly, one of the reasons for the trial (I never decided to really come back, yet, I just wanted to give Oz' project 'snowstorm' a fair chance) is that I wanted to avoid putting a lot of time into viewer 2, only to have wasted that time when the trial failed; so I wanted to avoid putting time into snowstorm when it the current state is unsatisfactory. Thus, I have been working on other projects for 10 weeks now(!) (not viewer or even SL related), and it doesn't seem that things are improving. Two weeks ago I informed Oz about my upcoming decision, that I will be forced to take, to leave the snowstorm project when nothing changed (and before that I let him know my dissatisfaction plenty of times of course). I said I'd stick around one more month (that's till the end of March). I'm not blaming anyone in particular - I'm sure that the snowstorm team is heavily undermanned - but the result is the same: this open source project is a failure. The only solution that I can think of given the current situation is to trust good coders more and given them (me included) direct write access to viewer-development, so I can work at my own pace. A given fact is that when I work at my own pace you can not keep up with reviewing and testing... you have to leave that to me and trust that I do a good job. We did that with snowglobe and as you know all those patches were good (in the end everything was ported from snowglobe to snowstorm, too). I simply cannot work like this: I want to work on a project that I CAN work on at my own (high) pace. I don't have a lot of hope anymore, but fair is fair... if those six patches are in viewer-development by the end of the month then I'll give it a bit more chance. This mail is just informative. Do with it what you want. Working currently many hours per day on http://m4ri.sagemath.org/ , Aleric ___ 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
Re: [opensource-dev] Review Request: make PREHASH variables char const* const (fixes VWR-24487: llurlentry_stub.cpp:196: error: deprecated conversion from string constant to 'char*')
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/#review429 --- Ship it! Perfect. - Aleric On March 7, 2011, 3:51 p.m., Boroondas Gupte wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/ --- (Updated March 7, 2011, 3:51 p.m.) Review request for Viewer and Seth ProductEngine. Summary --- For the reason for this change, see https://jira.secondlife.com/browse/VWR-24487 and https://jira.secondlife.com/browse/VWR-24522 What I did: In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant pointers to constants by search/replace. Then I tried to compile and added const qualifiers in dependent code as needed to stop the compiler complaining. Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files have been generated from the message template. Because this generation might not have been a one-off thing, I changed the generating code, too, so it won't override this change here when the generation happens the next time. However, that part of the code is not called by Viewer, although the relevant function — dump_prehash_files() — ends up in the Viewer binary. That function probably gets called by the simulator, when one runs the latter with -prehash. (See https://bitbucket.org/lindenlab/viewer-development/src/fc7e5dcf3059/indra/llmessage/message.cpp#cl-2532 ) This addresses bug VWR-24487. http://jira.secondlife.com/browse/VWR-24487 Diffs - doc/contributions.txt fc7e5dcf3059 indra/llmessage/message.cpp fc7e5dcf3059 indra/llmessage/message_prehash.h fc7e5dcf3059 indra/llmessage/message_prehash.cpp fc7e5dcf3059 indra/llprimitive/llprimitive.h fc7e5dcf3059 indra/llprimitive/llprimitive.cpp fc7e5dcf3059 indra/llprimitive/llvolumemessage.h fc7e5dcf3059 indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 Diff: http://codereview.secondlife.com/r/100/diff Testing --- Compiled (standalone, 64bit Linux) with and without LL_TESTS. Started the viewer, logged in, walked and flew around a bit. Everything seems to work. Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in indra/llui/tests/llurlentry_stub.cpp and indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually are never dereferenced, even when not NULL, so that using NULL pointers instead of place holder data won't change what code paths gets tested. Both tests binaries executed without crashes and all the contained tests passed. Locally invoked start_messaging_system() with b_dump_prehash_file == true instead of FALSE, to see what would be generated after my change to dump_prehash_files(). The message_prehash.(h|cpp) generated by that had the correct type qualifiers and formatting, but some lines were removed or added compared to the modified files from the source tree. That probably means that the files aren't fully synchronized with the message template file in the source tree. Because the added constants are spread all over the file, while the removed ones were at the end, I'd wager that message_prehash.(h|cpp) in the viewer source tree are actually more up-to-date than the message template file in the source tree. Thanks, Boroondas ___ 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
Re: [opensource-dev] Review Request: storm-1038: crash in texture cache pruning
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/172/#review416 --- You want to know what I think... https://codereview.secondlife.com/r/93/ has been waiting to finally be merged for 6 weeks now. - Aleric On March 3, 2011, 12:12 p.m., Oz Linden wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/172/ --- (Updated March 3, 2011, 12:12 p.m.) Review request for Viewer. Summary --- If the viewer is started when the maximum number of items allowed in the texture cache is greater than the number actually in the cache, the viewer crashes. This addresses bug storm-1038. http://jira.secondlife.com/browse/storm-1038 Diffs - indra/newview/lltexturecache.cpp 8596ccc522d9 Diff: http://codereview.secondlife.com/r/172/diff Testing --- Since I found this by somehow getting into the over-full condition when working on an unrelated issue, I found myself unable to launch my viewer. One this fix was applied, launching worked just fine. Thanks, Oz ___ 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
Re: [opensource-dev] Review Request: Nearby chat history is displaying both Display Names and user.names when the Display Name is not changed from default.
On Feb. 27, 2011, 5:57 a.m., Boroondas Gupte wrote:
indra/llcommon/llavatarname.cpp, lines 93-108
http://codereview.secondlife.com/r/153/diff/1/?file=981#file981line93
This could be shortened to
if (!mUsername.empty() !mIsDisplayNameDefault)
{
name = mDisplayName + ( + mUsername + );
}
else
{
// Display names are off, so legacy name is in
mDisplayName,
// or Display Name is not changed from (user name based)
// default, so also showing the user name would be
redundant.
name = mDisplayName;
}
but I guess the nested ifs make the intention more clear, so go for it.
Or even more readable(?):
if (mIsDisplayNameDefault || mUsername.empty())
{
// Showing the user name when Display Names are off
// is redundant, since in that case it's the same string.
name = mDisplayName;
}
else
{
name = mDisplayName + ( + mUsername + );
}
- Aleric
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/153/#review392
---
On Feb. 19, 2011, 9:32 a.m., ardy.lay wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/153/
---
(Updated Feb. 19, 2011, 9:32 a.m.)
Review request for Viewer.
Summary
---
https://jira.secondlife.com/browse/VWR-24917
I have been finding the redundent display of functionally equivalent names in
nearby chat history and IM history quite tiresome.
Simple proposal: If the resident's Display Name is at the default then do not
display their user.name.
https://bitbucket.org/ArdyLay/viewer-development-vwr-24917
Change is to: LLAvatarName::getCompleteName
I find the following Callers:
LLAvatarActions::requestFriendshipDialog
LLAvatarActions::startIM
LLAvatarActions::startCall
LLIMModel::LLIMSession
LLIMModel::logToFile
LLPostponedNotification::onAvatarNameCache
LLUrlEntryAgent::onAvatarNameCache
LLUrlEntryAgent::getLabel
LLUrlEntryAgentCompleteName::getName
// Callback for name resolution of a god/estate message
llviewermessage.cpp(2149): args[NAME] = av_name.getCompleteName();
llviewermessage.cpp(2154): chat.mText = av_name.getCompleteName() + : +
message;
static void on_avatar_name_cache_toast ...
llimview.cpp(108): args[FROM] = av_name.getCompleteName();
Some of these make me wonder if this change will cause some defects and
should be implimented as a seperate function.
This addresses bug VWR-24917.
http://jira.secondlife.com/browse/VWR-24917
Diffs
-
doc/contributions.txt c10d5e37db1e
indra/llcommon/llavatarname.cpp c10d5e37db1e
Diff: http://codereview.secondlife.com/r/153/diff
Testing
---
I have been using this trivial change and have shared it with a friend, via
bitbucket. We have both built the viewer on Windows 7 and find the resulting
reduction in redundent text in chat and IM history on screen to be very
helpful.
Thanks,
ardy.lay
___
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
Re: [opensource-dev] Review Request: OPEN-38: autobuild support for StandAlone
On Feb. 27, 2011, 3:47 a.m., Boroondas Gupte wrote: Ah, with that, configuration succeeds and GLH_INCLUDE_DIR gets correctly set to the dir with the symlinks. Could the need for that symlink dir be avoided by having the user directly set the GLH_INCLUDE_DIR CMake variable? There is long standing issue (from the days of Rob Linden) that I wanted to address once things finally get running smoothly... I don't think that now is the time to address this. What needs to be done for tut and glh_linear is to install them in an architecture independent directory and then use them for standalone and non-standalone. The same goes for the slvoice stuff. Some packages ARE needed even on standalone, but the current setup doesn't support it well enough because LL never tests standalone is therefore unaware of the difference between packages that are only needed for non-standalone and packages that are needed for both. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/167/#review391 --- On Feb. 26, 2011, 6:56 a.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/167/ --- (Updated Feb. 26, 2011, 6:56 a.m.) Review request for Viewer. Summary --- Oz: please merge into your repository. This addresses bug OPEN-38. Diffs - indra/cmake/FindGLH.cmake PRE-CREATION indra/cmake/GLH.cmake PRE-CREATION indra/cmake/LLRender.cmake 5f0ab9443ece indra/cmake/LLSharedLibs.cmake 5f0ab9443ece indra/cmake/Linking.cmake 5f0ab9443ece indra/linux_crash_logger/CMakeLists.txt 5f0ab9443ece Diff: http://codereview.secondlife.com/r/167/diff Testing --- configures, compiles, runs. Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: OPEN-38: autobuild support for StandAlone
On Feb. 27, 2011, 3:47 a.m., Boroondas Gupte wrote: indra/cmake/Linking.cmake, lines 6-20 http://codereview.secondlife.com/r/167/diff/1/?file=998#file998line6 I thought the removal of the NOT STANDALONE condition here would give standalone the same (repository-local) include directories that non-standalone has, so that it'd find autobuild-installed dependencies. But it seems those are just for the library binaries? Or is this just about the creation of those directories, not their addition to the search path? No, this just sets variables to certain (path) values. No test should ever test if they are set (that would be a wrong design), so they might as well be set. In fact, they ARE being used (at least some of them) on standalone (the staging directories), which is why I removed the if. Also, it's a Good Thing(tm) that the auto-build installed packages (and header files!) are completely invisible on standalone. This should not change. What we need, as I remarked in my other comment, is that we need to create a separate directory where packages can be installed that ARE needed on standalone and then use that, so that we ONLY get packages that we really want/need, and not left-overs from a non-standalone build as well). - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/167/#review391 --- On Feb. 26, 2011, 6:56 a.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/167/ --- (Updated Feb. 26, 2011, 6:56 a.m.) Review request for Viewer. Summary --- Oz: please merge into your repository. This addresses bug OPEN-38. Diffs - indra/cmake/FindGLH.cmake PRE-CREATION indra/cmake/GLH.cmake PRE-CREATION indra/cmake/LLRender.cmake 5f0ab9443ece indra/cmake/LLSharedLibs.cmake 5f0ab9443ece indra/cmake/Linking.cmake 5f0ab9443ece indra/linux_crash_logger/CMakeLists.txt 5f0ab9443ece Diff: http://codereview.secondlife.com/r/167/diff Testing --- configures, compiles, runs. Thanks, Aleric ___ 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
[opensource-dev] Review Request: OPEN-38: autobuild support for StandAlone
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/167/ --- Review request for Viewer. Summary --- Oz: please merge into your repository. This addresses bug OPEN-38. Diffs - indra/cmake/FindGLH.cmake PRE-CREATION indra/cmake/GLH.cmake PRE-CREATION indra/cmake/LLRender.cmake 5f0ab9443ece indra/cmake/LLSharedLibs.cmake 5f0ab9443ece indra/cmake/Linking.cmake 5f0ab9443ece indra/linux_crash_logger/CMakeLists.txt 5f0ab9443ece Diff: http://codereview.secondlife.com/r/167/diff Testing --- configures, compiles, runs. Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: OPEN-38: autobuild support for StandAlone
On Feb. 26, 2011, 2:44 p.m., Boroondas Gupte wrote: Can you give some instructions on how to do a standalone build with these changes? I tried the following: patch -p1 /home/das-g/slsrc/patches/autobuild-standalone.diff autobuild install glh_linear autobuild configure -c OpenSourceStandAloneRelWithDebInfo but this fails with: CMake Error at cmake/FindGLH.cmake:26 (message): Could not find GLH Thanks The usual way, you have to include the path where you installed your library to CMAKE_INCLUDE_PATH and CMAKE_LIBRARY_PATH. For GLH just CMAKE_INCLUDE_PATH is enough since it only exists of headers. hikaru:/usr/src/secondlife/viewers/snowstorm/viewer-autobuildecho $CMAKE_INCLUDE_PATH /usr/src/secondlife/llqtwebkit/install-imprudence/include:/usr/src/secondlife/viewers/snowstorm/viewer-autobuild/include:/sl/usr/include hikaru:/usr/src/secondlife/viewers/snowstorm/viewer-autobuildls -l /usr/src/secondlife/viewers/snowstorm/viewer-autobuild/include total 0 lrwxrwxrwx 1 aleric src 48 Feb 25 20:33 GL - ../linden/build-linux-x86_64/packages/include/GL/ lrwxrwxrwx 1 aleric src 49 Feb 25 20:33 glh - ../linden/build-linux-x86_64/packages/include/glh/ lrwxrwxrwx 1 aleric src 49 Feb 25 20:33 tut - ../linden/build-linux-x86_64/packages/include/tut/ As you see, I created symbolic links in some random directory (that I then added to CMAKE_INCLUDE_PATH) to whereever the respective packages are installed by an 'autobuild install'. I did that because if you add */packages/include to CMAKE_INCLUDE_PATH it finds ALL packages that you installed, which might not be what you want if you use the same source tree for standlone and non-standalone testing. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/167/#review388 --- On Feb. 26, 2011, 6:56 a.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/167/ --- (Updated Feb. 26, 2011, 6:56 a.m.) Review request for Viewer. Summary --- Oz: please merge into your repository. This addresses bug OPEN-38. Diffs - indra/cmake/FindGLH.cmake PRE-CREATION indra/cmake/GLH.cmake PRE-CREATION indra/cmake/LLRender.cmake 5f0ab9443ece indra/cmake/LLSharedLibs.cmake 5f0ab9443ece indra/cmake/Linking.cmake 5f0ab9443ece indra/linux_crash_logger/CMakeLists.txt 5f0ab9443ece Diff: http://codereview.secondlife.com/r/167/diff Testing --- configures, compiles, runs. Thanks, Aleric ___ 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
Re: [opensource-dev] Is 'STANDALONE' confusing?
paragraph to show its not that simple Regarding libraries and the second life viewer we have the following issues: The version of a library (source code + headers) at the moment it is compiled into a binary. In all cases we should only use THOSE headers whenever we compile anything that will link with that binary. This is not always the case, but that is a bug then, so lets leave that out of our consideration. Hence, for this analysis we condense the header and library versions into one: the version of the library (and it's headers) that is being compiled, and are being used when compiling. This version is not necessarily the same as the runtime version of a library (where headers do not come into play at all). Furthermore we may assume that internally Linden Lab uses the same versions (that is, they compile a viewer and possibly other libraries, against the same version as that they ship as prebuilt with that viewer). However, there is an interdependency between libraries: some libraries are compiled against other libraries (and both need to be shipped). Again theoretically it would be possible that a TPV releases prebuilt libraries for some libraries but not for all: using the LL prebuilt for some. Also here I will assume that if a TPV replaces some libraries, then they will at least replace also all libraries that depend on that library or use that library, so that we can ignore library interdependencies mostly. Mostly, because we cannot ignore those dependencies for libraries that are never shipped as prebuilt: it is possible that LL compiles it's prebuilt libs against libc 2.9, some TPV compiles some prebuilts against libc 2.10 and the user that compiles her own compiler uses libc 2.11. Theoretically this can lead to problems, but that is unlikely when it concerns C libraries. Thus, as long as every C++ library is a prebuilt then we can probably ignore this anyway. Plus we probably should ignore the existance of TPV's anyway here :p Having simplified things thus, we end up with: LL prebuilt version (assumed to be shipped WITH headers) System library version (with headers in it's -dev package), or User compiled version (libraries not provided by the standard distribution). If we built 'standalone' then it's simple: we compile everything using the same version of any particular library (all libraries are compiled against eachother: the same version was used even when compiling the system libs (and should have been used while compiling the user compiled libraries). Also here an exception: if you do not compile your own libraries, but use for example Michelle's debian repository for those libraries that are not distributed with the stock debian, then those libraries might be compiled against different versions. We also ignore this since we trust that Michelle makes sure can ignore that. If we DO use the prebuilt libraries it is possible that we compile against headers that are newer than what LL used, and link with same, newer versions, at runtime. This usually shouldn't be a problem, and when it is it should be detected by one of the developers and fixed in the viewer code (ie, developers using a newer version of boost asked for viewer code fixes that are backwards compatible and cause things to work in such a case). /paragraph to show its not that simple Now to answer your question: the above is complex, even with a lot of simplification and ignoring stuff. Calling everything that is not a Linden Lab prebuilt a system library doesn't cut it. The only really clear constant in all of the above is what prebuilt means, especially Linden Lab prebuilt. Therefore I think it's better to use PREBUILT as keyword than SYSTEM_LIBS. On Mon, Feb 21, 2011 at 4:46 PM, Boroondas Gupte slli...@boroon.dasgupta.ch wrote: On 02/21/2011 04:41 PM, Aleric Inglewood wrote: A LOT worse, but still better than 'standalone' would be USE_SYSTEM_LIBS. Why would you consider USE_SYSTEM_LIBS worse than other suggestions? Cheers, Boroondas ___ 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 ___ 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
Re: [opensource-dev] Review Request: make PREHASH variables char const* const
On Feb. 3, 2011, 6:35 p.m., Aleric Inglewood wrote: Can this patch please be added to viewer-development? It's getting really annoying that I have to apply patches to the soruce tree before it even can compile cleanly :(. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/#review321 --- On Jan. 22, 2011, 7:40 a.m., Boroondas Gupte wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/ --- (Updated Jan. 22, 2011, 7:40 a.m.) Review request for Viewer and Seth ProductEngine. Summary --- For the reason for this change, see https://jira.secondlife.com/browse/VWR-24487 and https://jira.secondlife.com/browse/VWR-24522 What I did: In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant pointers to constants by search/replace. Then I tried to compile and added const qualifiers in dependent code as needed to stop the compiler complaining. Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files have been generated from the message template. Because this generation might not have been a one-off thing, I changed the generating code, too, so it won't override this change here when the generation happens the next time. However, that part of the code is not called by Viewer, although the relevant function — dump_prehash_files() — ends up in the Viewer binary. That function probably gets called by the simulator, when one runs the latter with -prehash. (See https://bitbucket.org/lindenlab/viewer-development/src/fc7e5dcf3059/indra/llmessage/message.cpp#cl-2532 ) This addresses bug VWR-24487. http://jira.secondlife.com/browse/VWR-24487 Diffs - doc/contributions.txt fc7e5dcf3059 indra/llmessage/message.cpp fc7e5dcf3059 indra/llmessage/message_prehash.h fc7e5dcf3059 indra/llmessage/message_prehash.cpp fc7e5dcf3059 indra/llprimitive/llprimitive.h fc7e5dcf3059 indra/llprimitive/llprimitive.cpp fc7e5dcf3059 indra/llprimitive/llvolumemessage.h fc7e5dcf3059 indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 Diff: http://codereview.secondlife.com/r/100/diff Testing --- Compiled (standalone, 64bit Linux) with and without LL_TESTS. Started the viewer, logged in, walked and flew around a bit. Everything seems to work. Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in indra/llui/tests/llurlentry_stub.cpp and indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually are never dereferenced, even when not NULL, so that using NULL pointers instead of place holder data won't change what code paths gets tested. Both tests binaries executed without crashes and all the contained tests passed. Locally invoked start_messaging_system() with b_dump_prehash_file == true instead of FALSE, to see what would be generated after my change to dump_prehash_files(). The message_prehash.(h|cpp) generated by that had the correct type qualifiers and formatting, but some lines were removed or added compared to the modified files from the source tree. That probably means that the files aren't fully synchronized with the message template file in the source tree. Because the added constants are spread all over the file, while the removed ones were at the end, I'd wager that message_prehash.(h|cpp) in the viewer source tree are actually more up-to-date than the message template file in the source tree. Thanks, Boroondas ___ 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
Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)
On Jan. 31, 2011, 6:44 p.m., Merov Linden wrote: @Aleric: OK, you convinced me on all accounts *except* for the ease of merge which I want to test myself before giving my blessing to this patch. You wouldn't be the first one be to claim that it merges and create some issues unwittingly. If you could post the URL of an hg repo with your patch, I'll be happy to give it a spin building on all platforms. BTW, thanks for the super detailed comment: very interesting stuff. Thanks. I made a repository available here: https://bitbucket.org/aleric/viewer-development-storm-955 Due to technical reasons I had to base it on a recent viewer-development commit, but during the merge I had no collisions except in contributions.txt ;) (while I made the original patch a few hunderd commits ago). [PS I added this TWO DAYS ago... but forgot to click on 'Publish'... this kinda sucks a bit] - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/81/#review300 --- On Jan. 16, 2011, 5:53 a.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/81/ --- (Updated Jan. 16, 2011, 5:53 a.m.) Review request for Viewer. Summary --- Turns out that most of my SNOW-800 patch was included in Viewer 2 (albeit without crediting me). However, not everything was used and some more cleaning up was possible. After this patch, and when compiling with optimization, there are no duplicates left anymore that shouldn't be there in the first place: apart from the debug stream iostream guard variable, there are several static variables with the same name (r, r1, r2, etc) but that indeed actually different symbol objects. Then there are a few constant POD arrays that are duplicated a hand full of times because they are accessed with a variable index (so optimizing them away is not possible). I left them like that (although defining those as extern as well would have been more consistent and not slower; in fact it would be faster theoretically because those arrays could share the same cache page then). This addresses bug VWR-24312. http://jira.secondlife.com/browse/VWR-24312 Diffs - doc/contributions.txt 422f636c3343 indra/llcharacter/llanimationstates.cpp 422f636c3343 indra/llcommon/llavatarconstants.h 422f636c3343 indra/llcommon/lllslconstants.h 422f636c3343 indra/llcommon/llmetricperformancetester.h 422f636c3343 indra/llmath/llcamera.h 422f636c3343 indra/llmath/llcamera.cpp 422f636c3343 indra/newview/llviewerobject.cpp 422f636c3343 indra/newview/llvoavatar.cpp 422f636c3343 indra/newview/llvosky.h 422f636c3343 indra/newview/llvosky.cpp 422f636c3343 Diff: http://codereview.secondlife.com/r/81/diff Testing --- Compiled with optimization and then running readelf on the executable to find duplicated symbols. Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: STORM-864: As as developer, I would like an object oriented wrapper to make safe use of memory pools easier
On Feb. 2, 2011, 6:30 p.m., Merov Linden wrote: indra/llcommon/llaprpool.h, lines 107-116 http://codereview.secondlife.com/r/99/diff/2/?file=676#file676line107 I think it's be more clear to use an explicit name like getAPRPool() rather than the function operator here. It produces hard to read code in a couple of places and it's just plain unclear what myPool() really does. That is actually on purpose. Developers should never actually get the pool. Like the comment says, it's painful that we have to provide access at all: we want to hide apt_pool_t completely from the user. The user should use LLAPRPool instead. And then, when that has to be passed to the actual libapr function, it's passed like this: apr_whatever_create(foo, mPool()); because that is just a little bit more safe than have it auto convert and allow writing apr_whatever_create(foo, mPool); or so I thought. It's deliberately a bit weird, to make people think twice before they get the underlaying apr_pool_t. Also... see comment below... - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/99/#review315 --- On Jan. 29, 2011, 9:10 a.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/99/ --- (Updated Jan. 29, 2011, 9:10 a.m.) Review request for Viewer. Summary --- Please see http://jira.secondlife.com/browse/STORM-864 I made a mercurial repository available for testing here: hg clone https://bitbucket.org/aleric/viewer-development-storm-864 From the commit message: Introduces a LLThreadLocalData class that can be accessed through the static LLThread::tldata(). Currently this object contains two (public) thread-local objects: a LLAPRRootPool and a LLVolatileAPRPool. The first is the general memory pool used by this thread (and this thread alone), while the second is intended for short lived memory allocations (needed for APR). The advantages of not mixing those two is that the latter is used most frequently, and as a result of it's nature can be destroyed and reconstructed on a regular basis. This patch adds LLAPRPool (completely replacing the old one), which is a wrapper around apr_pool_t* and has complete thread-safity checking. Whenever an apr call requires memory for some resource, a memory pool in the form of an LLAPRPool object can be created with the same life-time as this resource; assuring clean up of the memory no sooner, but also not much later than the life-time of the resource that needs the memory. Many, many function calls and constructors had the pool parameter simply removed (it is no longer the concern of the developer, if you don't write code that actually does an libapr call then you are no longer bothered with memory pools at all). However, I kept the notion of short-lived and long-lived allocations alive (see my remark in the jira here: https://jira.secondlife.com/browse/STORM-864?focusedCommentId=235356page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-235356 which requires that the LLAPRFile API needs to allow the user to specify how long they think a file will stay open. By choosing 'short_lived' as default for the constructor that immediately opens a file, the number of instances where this needs to be specified is drastically reduced however (obviously, any automatic LLAPRFile is short lived). This addresses bug STORM-864. http://jira.secondlife.com/browse/STORM-864 Diffs - doc/contributions.txt fe7fe04ccc9a indra/llaudio/llaudioengine_fmod.cpp fe7fe04ccc9a indra/llaudio/llvorbisencode.cpp fe7fe04ccc9a indra/llcharacter/llbvhloader.cpp fe7fe04ccc9a indra/llcharacter/llkeyframemotionparam.cpp fe7fe04ccc9a indra/llcharacter/llstatemachine.cpp fe7fe04ccc9a indra/llcommon/CMakeLists.txt fe7fe04ccc9a indra/llcommon/llapp.cpp fe7fe04ccc9a indra/llcommon/llapr.h fe7fe04ccc9a indra/llcommon/llapr.cpp fe7fe04ccc9a indra/llcommon/llaprpool.h PRE-CREATION indra/llcommon/llaprpool.cpp PRE-CREATION indra/llcommon/llcommon.h fe7fe04ccc9a indra/llcommon/llcommon.cpp fe7fe04ccc9a indra/llcommon/llerror.h fe7fe04ccc9a indra/llcommon/llerror.cpp fe7fe04ccc9a indra/llcommon/llfixedbuffer.cpp fe7fe04ccc9a indra/llcommon/llscopedvolatileaprpool.h PRE-CREATION indra/llcommon/llthread.h fe7fe04ccc9a indra/llcommon/llthread.cpp fe7fe04ccc9a indra/llcommon/llthreadsafequeue.h fe7fe04ccc9a indra/llcommon/llthreadsafequeue.cpp fe7fe04ccc9a indra/llcommon/llworkerthread.h fe7fe04ccc9a indra/llcommon/llworkerthread.cpp fe7fe04ccc9a indra
[opensource-dev] [linux] /usr/bin/ld: error: unsupported symbol binding
If you are using linux and get this error while compiling the viewer: /usr/bin/ld: error: unsupported symbol binding (or, for that matter: /usr/bin/gold: error: unsupported symbol binding then you are using ld.gold version 2.20.x. You need to upgrade it to 2.21. If you are using debian then you can get that from experimental (but only if you know what you're doing ;) .. don't upgrade your whole system to experimental by accident!), as follows: To /etc/apt/preferences add: Package: * Pin: release a=testing Pin-Priority: 990 where 'testing' is YOUR CURRENT RELEASE! So, replace it if you are not using testing. This preference is to avoid that you upgrade your whole system: it will prefer 'testing' or whatever you use over the rest. Next, add to /etc/apt/sources.list an entry for experimental: deb http://ftp.nl.debian.org/debian/ experimental main contrib non-free deb-src http://ftp.nl.debian.org/debian/ experimental main contrib non-free replace ftp.nl.debian.org with your own favourite debian server. Run apt-get update to get the info from this new entry. Now you could try apt-cache policy binutils To see that it would NOT upgrade your binutils because of the preferences ;). Next, you can upgrade /usr/bin/gold with: sudo apt-get install -t experimental binutils I only tried this on testing though. If you have an older distribution it might want to upgrade more than just binutils. If that's the case then don't do it. Please note that if you'd install binutils-gold (which is on experimental) then that will remove your nvidia kernel module because that is not compatible with ld.gold. You don't want that ;). However, just upgrading binutils is ok, and then use LDFLAGS=-Wl,-use-gold as before to cause the viewer compilation to use gold. Aleric PS I used the same /etc/apt preferences and /etc/apt/sources.list to get my nvidia driver from experimental. As root: update-pciids apt-get install module-assistant apt-get install -t experimental nvidia-kernel-source m-a prepare m-a clean nvidia m-a a-i nvidia apt-get install -t experimental nvidia-glx depmod -a modprobe nvidia which gives me 260.19.21which supports my video card, while testing still has the old 195.36.31that does not support it. ___ 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
Re: [opensource-dev] Review Request: STORM-864: As as developer, I would like an object oriented wrapper to make safe use of memory pools easier
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/99/ --- (Updated Jan. 29, 2011, 9:10 a.m.) Review request for Viewer. Changes --- This new diff incorporates the changes as discussed above, and is updated to be based on revision fe7fe04ccc9a (Fri Jan 28 21:23:35 2011 -0700). I also added comments in this version on every line that correctly uses apr_pool_t, so it's easy to just do a grep on apr_pool_t and check if nobody accidently used that instead of LLAPRPool (in the future). I made this patch (again) available as mercurial repository, here: hg clone https://bitbucket.org/aleric/viewer-development-storm-864 (that is the same name as before, but be warned that I destroyed the old one and created a new repository). Summary --- Please see http://jira.secondlife.com/browse/STORM-864 I made a mercurial repository available for testing here: hg clone https://bitbucket.org/aleric/viewer-development-storm-864 From the commit message: Introduces a LLThreadLocalData class that can be accessed through the static LLThread::tldata(). Currently this object contains two (public) thread-local objects: a LLAPRRootPool and a LLVolatileAPRPool. The first is the general memory pool used by this thread (and this thread alone), while the second is intended for short lived memory allocations (needed for APR). The advantages of not mixing those two is that the latter is used most frequently, and as a result of it's nature can be destroyed and reconstructed on a regular basis. This patch adds LLAPRPool (completely replacing the old one), which is a wrapper around apr_pool_t* and has complete thread-safity checking. Whenever an apr call requires memory for some resource, a memory pool in the form of an LLAPRPool object can be created with the same life-time as this resource; assuring clean up of the memory no sooner, but also not much later than the life-time of the resource that needs the memory. Many, many function calls and constructors had the pool parameter simply removed (it is no longer the concern of the developer, if you don't write code that actually does an libapr call then you are no longer bothered with memory pools at all). However, I kept the notion of short-lived and long-lived allocations alive (see my remark in the jira here: https://jira.secondlife.com/browse/STORM-864?focusedCommentId=235356page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-235356 which requires that the LLAPRFile API needs to allow the user to specify how long they think a file will stay open. By choosing 'short_lived' as default for the constructor that immediately opens a file, the number of instances where this needs to be specified is drastically reduced however (obviously, any automatic LLAPRFile is short lived). This addresses bug STORM-864. http://jira.secondlife.com/browse/STORM-864 Diffs (updated) - doc/contributions.txt fe7fe04ccc9a indra/llaudio/llaudioengine_fmod.cpp fe7fe04ccc9a indra/llaudio/llvorbisencode.cpp fe7fe04ccc9a indra/llcharacter/llbvhloader.cpp fe7fe04ccc9a indra/llcharacter/llkeyframemotionparam.cpp fe7fe04ccc9a indra/llcharacter/llstatemachine.cpp fe7fe04ccc9a indra/llcommon/CMakeLists.txt fe7fe04ccc9a indra/llcommon/llapp.cpp fe7fe04ccc9a indra/llcommon/llapr.h fe7fe04ccc9a indra/llcommon/llapr.cpp fe7fe04ccc9a indra/llcommon/llaprpool.h PRE-CREATION indra/llcommon/llaprpool.cpp PRE-CREATION indra/llcommon/llcommon.h fe7fe04ccc9a indra/llcommon/llcommon.cpp fe7fe04ccc9a indra/llcommon/llerror.h fe7fe04ccc9a indra/llcommon/llerror.cpp fe7fe04ccc9a indra/llcommon/llfixedbuffer.cpp fe7fe04ccc9a indra/llcommon/llscopedvolatileaprpool.h PRE-CREATION indra/llcommon/llthread.h fe7fe04ccc9a indra/llcommon/llthread.cpp fe7fe04ccc9a indra/llcommon/llthreadsafequeue.h fe7fe04ccc9a indra/llcommon/llthreadsafequeue.cpp fe7fe04ccc9a indra/llcommon/llworkerthread.h fe7fe04ccc9a indra/llcommon/llworkerthread.cpp fe7fe04ccc9a indra/llcrashlogger/llcrashlogger.cpp fe7fe04ccc9a indra/llimage/llimage.cpp fe7fe04ccc9a indra/llimage/llimagedimensionsinfo.cpp fe7fe04ccc9a indra/llimage/llimagej2c.cpp fe7fe04ccc9a indra/llimage/llimageworker.h fe7fe04ccc9a indra/llimage/llimageworker.cpp fe7fe04ccc9a indra/llmath/llvolumemgr.cpp fe7fe04ccc9a indra/llmessage/llares.cpp fe7fe04ccc9a indra/llmessage/llcurl.cpp fe7fe04ccc9a indra/llmessage/lliohttpserver.h fe7fe04ccc9a indra/llmessage/lliohttpserver.cpp fe7fe04ccc9a indra/llmessage/lliosocket.h fe7fe04ccc9a indra/llmessage/lliosocket.cpp fe7fe04ccc9a indra/llmessage/llmail.h fe7fe04ccc9a indra/llmessage/llmail.cpp fe7fe04ccc9a indra/llmessage/llpumpio.h fe7fe04ccc9a indra/llmessage/llpumpio.cpp fe7fe04ccc9a
Re: [opensource-dev] Review Request: make PREHASH variables char const* const
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/#review245 --- Ship it! Latest diff looks good. Great that you found the generator for this code! - Aleric On Jan. 22, 2011, 7:40 a.m., Boroondas Gupte wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/ --- (Updated Jan. 22, 2011, 7:40 a.m.) Review request for Viewer and Seth ProductEngine. Summary --- For the reason for this change, see https://jira.secondlife.com/browse/VWR-24487 and https://jira.secondlife.com/browse/VWR-24522 What I did: In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant pointers to constants by search/replace. Then I tried to compile and added const qualifiers in dependent code as needed to stop the compiler complaining. Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files have been generated from the message template. Because this generation might not have been a one-off thing, I changed the generating code, too, so it won't override this change here when the generation happens the next time. However, that part of the code is not called by Viewer, although the relevant function — dump_prehash_files() — ends up in the Viewer binary. That function probably gets called by the simulator, when one runs the latter with -prehash. (See https://bitbucket.org/lindenlab/viewer-development/src/fc7e5dcf3059/indra/llmessage/message.cpp#cl-2532 ) This addresses bug VWR-24487. http://jira.secondlife.com/browse/VWR-24487 Diffs - doc/contributions.txt fc7e5dcf3059 indra/llmessage/message.cpp fc7e5dcf3059 indra/llmessage/message_prehash.h fc7e5dcf3059 indra/llmessage/message_prehash.cpp fc7e5dcf3059 indra/llprimitive/llprimitive.h fc7e5dcf3059 indra/llprimitive/llprimitive.cpp fc7e5dcf3059 indra/llprimitive/llvolumemessage.h fc7e5dcf3059 indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 Diff: http://codereview.secondlife.com/r/100/diff Testing --- Compiled (standalone, 64bit Linux) with and without LL_TESTS. Started the viewer, logged in, walked and flew around a bit. Everything seems to work. Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in indra/llui/tests/llurlentry_stub.cpp and indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually are never dereferenced, even when not NULL, so that using NULL pointers instead of place holder data won't change what code paths gets tested. Both tests binaries executed without crashes and all the contained tests passed. Locally invoked start_messaging_system() with b_dump_prehash_file == true instead of FALSE, to see what would be generated after my change to dump_prehash_files(). The message_prehash.(h|cpp) generated by that had the correct type qualifiers and formatting, but some lines were removed or added compared to the modified files from the source tree. That probably means that the files aren't fully synchronized with the message template file in the source tree. Because the added constants are spread all over the file, while the removed ones were at the end, I'd wager that message_prehash.(h|cpp) in the viewer source tree are actually more up-to-date than the message template file in the source tree. Thanks, Boroondas ___ 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
Re: [opensource-dev] Review Request: VWR-24337: Possible crash on llassert_always(purge_list.size() = entries_to_purge)
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/93/ --- (Updated Jan. 21, 2011, 4:25 a.m.) Review request for Viewer. Changes --- The new diff uses while(purge_list.size() entries_to_purge), instead of a for() loop. Summary --- Just fixed the logic, so entries_to_purge won't become negative anymore, and the rest. This addresses bug VWR-24337. http://jira.secondlife.com/browse/VWR-24337 Diffs (updated) - doc/contributions.txt 422f636c3343 indra/newview/lltexturecache.cpp 422f636c3343 Diff: http://codereview.secondlife.com/r/93/diff Testing --- Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)
On Jan. 14, 2011, 1:31 p.m., Boroondas Gupte wrote:
indra/llcommon/lllslconstants.h, line 184
http://codereview.secondlife.com/r/81/diff/1/?file=392#file392line184
Yay for having type modifiers after the core type name. Makes them much
easier to understand, especially when there are several cascaded ones. :-)
Aleric Inglewood wrote:
Yeah, I'm strongly convinced that TYPE const is superior in anyway over
const TYPE.
See http://www.xs4all.nl/~carlo17/cpp/const.qualifier.html for the
reasoning.
In one line: all type qualifiers work to the left, it's best to PUT them
on the
left so the whole type is logically uniform in it's construction. The
fact that
you can legally type 'const TYPE' is just a historically grown misfortune.
Of course I meant.. All type qualifiers work to the left, so it's best to PUT
them on the _right_ ..., as in:
TYPE QUALIFIER : The Qualifier changes the TYPE on it's left, so that what
first was TYPE now becomes TYPE QUALIFIER.
[Where QUALIFIER is not just const, volatile, restrict, but also * and .
The only exception where any qualifier works to the right is where 'const'
(volatile and restrict, but NOT * an ) works on a base type. Surely it needs
getting used when one changes style, but this one is so logical that already
after a single week you don't understand anymore how you were ever able to
use to previous format style.
While on the topic of coding style and types. The above is a good reason to
put * (and ) that are part of types *against* the TYPE they work on.
That way one can easily recognize the difference between the unary operator,
the binary operator and the type qualifier:
A* b = *c * d + e; // The type qualifier has no space on it's left (and
// changes the type A), the binary operator (multiplication)
// has spaces on both sides, while the unary operator
// (dereference) works to it's right side and has no
// space on the right.]
[[ Ie,
#include iostream
struct A { int i; };
int main()
{
A const a[7] = { 0, 1, 2, 3, 4, 5, 6 };
int const two = 2;
int const* c = two;
int const d = 3;
A* const e = a[0];
A* b = *c * d + e;
std::cout b-i std::endl;
}
]]
- Aleric
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/81/#review153
---
On Jan. 16, 2011, 5:53 a.m., Aleric Inglewood wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/81/
---
(Updated Jan. 16, 2011, 5:53 a.m.)
Review request for Viewer.
Summary
---
Turns out that most of my SNOW-800 patch was included in Viewer 2 (albeit
without crediting me).
However, not everything was used and some more cleaning up was possible.
After this patch, and when compiling with optimization, there are no
duplicates left
anymore that shouldn't be there in the first place: apart from the debug
stream
iostream guard variable, there are several static variables with the same
name (r, r1,
r2, etc) but that indeed actually different symbol objects. Then there are a
few
constant POD arrays that are duplicated a hand full of times because they are
accessed with a variable index (so optimizing them away is not possible). I
left them
like that (although defining those as extern as well would have been more
consistent
and not slower; in fact it would be faster theoretically because those arrays
could
share the same cache page then).
This addresses bug VWR-24312.
http://jira.secondlife.com/browse/VWR-24312
Diffs
-
doc/contributions.txt 422f636c3343
indra/llcharacter/llanimationstates.cpp 422f636c3343
indra/llcommon/llavatarconstants.h 422f636c3343
indra/llcommon/lllslconstants.h 422f636c3343
indra/llcommon/llmetricperformancetester.h 422f636c3343
indra/llmath/llcamera.h 422f636c3343
indra/llmath/llcamera.cpp 422f636c3343
indra/newview/llviewerobject.cpp 422f636c3343
indra/newview/llvoavatar.cpp 422f636c3343
indra/newview/llvosky.h 422f636c3343
indra/newview/llvosky.cpp 422f636c3343
Diff: http://codereview.secondlife.com/r/81/diff
Testing
---
Compiled with optimization and then running readelf on the executable to find
duplicated symbols.
Thanks,
Aleric
___
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
Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)
from a header to the .cpp file), but for some reason this change wasn't copied to viewer 2. I have no objections whatsoever if you decide not to apply this hunk, but seriously - how often is THAT part of the code changed by other people? It's a list of UUID's for crying out loud. It will NOT be changed for MONTHS to come. It will really not cause any collisions, or merge horror, because nobody else changes that code, ever. 2) Four lines in indra/llcommon/llavatarconstants.h that gets rid of the VISIBILITY_* duplications. Surely those four lines won't give you a merge problem either. It's extremely unlikely that, until everyone merged this, anyone will change code near those lines, and even if some person does - the fix of such collision would be extremely trivial. Likewise one line in indra/llcommon/lllslconstants.h for TEXTBOX_MAGIC_TOKEN. One line in indra/llcommon/llmetricperformancetester.h for DEFAULT_METRIC_NAME etc... minimal changes that in practise are very unlikely to even cause a collision before this is merged to everyone, and are faster to fix if it happens than it took you to read this post. 3) A serious code cleanup (and getting rid of the duplicates) regarding the AIR_SCA (AirSca) calculations. I say serious, because the old code is hardly maintainable and very unclear. If you have a hard time reviewing it then I'm sure that is because it nearly impossible to follow what the OLD code did... Again, yes, the old code worked... so why apply the patch when there is a risk for collisions? Well, in this case it's my opinion that the old code is SO bad-- that now the work of cleaning it up is done-- it's worth the risk... especially because in the end, it only changes things very locally (indra/newview/llvosky.cpp), again not likely at all that anyone else will be making changes to that code right now (especially since they'd need to understand the code first - heheh, highly unlikely!). Let me know what your decision is, so I can adapt my final test repository of the collection of the VWR* jiras that I currently have pending on the review board. Thanks for reading (sorry it was so long), Aleric - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/81/#review221 --- On Jan. 16, 2011, 5:53 a.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/81/ --- (Updated Jan. 16, 2011, 5:53 a.m.) Review request for Viewer. Summary --- Turns out that most of my SNOW-800 patch was included in Viewer 2 (albeit without crediting me). However, not everything was used and some more cleaning up was possible. After this patch, and when compiling with optimization, there are no duplicates left anymore that shouldn't be there in the first place: apart from the debug stream iostream guard variable, there are several static variables with the same name (r, r1, r2, etc) but that indeed actually different symbol objects. Then there are a few constant POD arrays that are duplicated a hand full of times because they are accessed with a variable index (so optimizing them away is not possible). I left them like that (although defining those as extern as well would have been more consistent and not slower; in fact it would be faster theoretically because those arrays could share the same cache page then). This addresses bug VWR-24312. http://jira.secondlife.com/browse/VWR-24312 Diffs - doc/contributions.txt 422f636c3343 indra/llcharacter/llanimationstates.cpp 422f636c3343 indra/llcommon/llavatarconstants.h 422f636c3343 indra/llcommon/lllslconstants.h 422f636c3343 indra/llcommon/llmetricperformancetester.h 422f636c3343 indra/llmath/llcamera.h 422f636c3343 indra/llmath/llcamera.cpp 422f636c3343 indra/newview/llviewerobject.cpp 422f636c3343 indra/newview/llvoavatar.cpp 422f636c3343 indra/newview/llvosky.h 422f636c3343 indra/newview/llvosky.cpp 422f636c3343 Diff: http://codereview.secondlife.com/r/81/diff Testing --- Compiled with optimization and then running readelf on the executable to find duplicated symbols. Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: make PREHASH variables char const* const
On Jan. 20, 2011, 3:54 a.m., Boroondas Gupte wrote: I agree though, that we should try to handle this similarly in both tests, if possible. So I tried setting the pointers in indra/llui/tests/llurlentry_stub.cpp to NULL, too, which works nicely. However, I then realized that the tested code might have nullpointer guards around dereferencing code, so that if we pass nullpointers we are actually testing another scenario than when passing pointers to actual data and that this might be the reason why no null-pointer exceptions occur during the tests. Of course, we could now examine the tested code to see whether this is the case. However, it would be a bad idea to adapt the test code based on that examination, as the tested code might be changed later without this question being re-evaluated and the test rewritten accordingly. Is there another pointer value besides NULL (thus not usually tested for) that still fails reliably when tried to dereference? [*] Remember that tests are executed when building and are hopefully completely deterministic, so that any potential null-pointer-exception would have shown up. Sure set them to (char const*)0x1; That is not false, and surely will crash if you try to access it (it's odd). I'd only do this once for a local test though. If it still runs fine, you can set them to NULL and add a comment that they're not used. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/#review207 --- On Jan. 18, 2011, 7:29 a.m., Boroondas Gupte wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/ --- (Updated Jan. 18, 2011, 7:29 a.m.) Review request for Viewer. Summary --- For the reason for this change, see https://jira.secondlife.com/browse/VWR-24487 and https://jira.secondlife.com/browse/VWR-24522 What I did: In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant pointers to constants by search/replace. Then I tried to compile and added const qualifiers in dependent code as needed to stop the compiler complaining. Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files have been generated from the message template. If this generation wasn't a one-off thing, the generating code must be changed, too, so it won't override this change here when the generation happens the next time. This addresses bug VWR-24487. http://jira.secondlife.com/browse/VWR-24487 Diffs - doc/contributions.txt fc7e5dcf3059 indra/llmessage/message_prehash.h fc7e5dcf3059 indra/llmessage/message_prehash.cpp fc7e5dcf3059 indra/llprimitive/llprimitive.h fc7e5dcf3059 indra/llprimitive/llprimitive.cpp fc7e5dcf3059 indra/llprimitive/llvolumemessage.h fc7e5dcf3059 indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 Diff: http://codereview.secondlife.com/r/100/diff Testing --- Compiled (standalone, 64bit Linux) with and without LL_TESTS. Started the viewer, logged in, walked and flew around a bit. Everything seems to work. Thanks, Boroondas ___ 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
Re: [opensource-dev] Review Request: VWR-24321: Validate textures starting with 00 too.
On Jan. 18, 2011, 12:09 p.m., Merov Linden wrote: indra/newview/lltexturecache.cpp, line 1595 http://codereview.secondlife.com/r/90/diff/1/?file=413#file413line1595 validate_idx being used in a test later, it's not just for (validate_idx == 0) that the behavior will be different. I need to understand better what that idx is all about or you need to give a bit more explanation before I approve this diff. Aleric Inglewood wrote: The debug setting CacheValidateCounter is set to 'next_id', which makes clear what it's meaning is: namely, the id that we will check next time. next_idx is a very local variable that is simply set to the value of CacheValidateCounter plus 1, and then that value is stored to CacheValidateCounter again for next time. validate_idx is the ID that is actually being tested this time. Hence, it should be the value of CacheValidateCounter before we increase that. Obviously, those ID's should be in the range 0...255, which is garanteed by doing a modulo 256 on next_id before writing it to CacheValidateCounter. The old code also increases validate_idx *before* it is used. That means that it will be in the range 1...256, and 0 is never tested (note that validate_idx is just increased, there is no modulo applied, so it's obvious that it shouldn't be increased). Even the wording (next_id) suggests that validate_idx should just be the value of CacheValidateCounter, which is the value of the previous next_id... So, after this patch, we get to the following code with validate_idx in the range 0...255, as it should be. Cron Stardust wrote: Just double checking, as switching from pre-increment to addition can change behavior: In both cases next_idx will have the same value after this line is executed, however validate_idx will not. Prediff start state: validate_idx == 0; Prediff stop state: validate_idx == 1; next_idx == 1; Postdiff start state: validate_idx == 0; Postdiff stop state: validate_idx == 0; next_idx == 1; And another round over at the other end: Prediff start state: validate_idx == 255; Prediff stop state: validate_idx == 256; next_idx == 0; Postdiff start state: validate_idx == 255; Postdiff stop state: validate_idx == 255; next_idx == 0; So, yes, validate_idx will only have a 255 in it in this last case, however the over-all behavior has changed: validate_idx isn't being incremented at all in this line. WARNING: I have NOT looked at the rest of the diff, only at this one line. Nor do I know if validate_idx should or shouldn't be incremented by this line of code. Twisted Laws wrote: Given the way this seems to work to me without changing the sequence things are checked... I'd change line 1619 to if (uuididx == (validate_idx % 256)) Otherwise it was checking for 1 thru 256 and never 0... this does not change what appears to be incorrect coding where Aleric pointed out and thus won't change the current logic that it starts by checking 1 thru 255 before checking 0. To retain the current sequence that things are checked, you would have to compare uuididx to next_idx along with the change Aleric provided. However it seems to me that all is ok with using Aleric's correction and leaving the remaining code untouched. (I can't see where changing the sequence of checking makes a difference.) Boroondas Gupte wrote: Just double checking, as switching from pre-increment to addition can change behavior: In both cases next_idx will have the same value after this line is executed, however validate_idx will not. That's the intention. Without the change suggested here, validate_idx ends up in the wrong range. See my more detailed review below. @Cron Stardust : That is correct. The bug was that validate_idx was 1 too large. Nor incrementing it is the correct thing. Re the 'WARNING': this line is the only line in the diff :p - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/90/#review191 --- On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/90/ --- (Updated Jan. 14, 2011, 1:02 p.m.) Review request for Viewer. Summary --- Trivial patch, just removes stupidity. This addresses bug VWR-24321. http://jira.secondlife.com/browse/VWR-24321 Diffs - doc/contributions.txt b0bd26c5638a indra/newview/lltexturecache.cpp b0bd26c5638a Diff: http
Re: [opensource-dev] Link times
I can confirm that. I'm using a RAM disk (0.08ms random access time) and that does indeed drop link time significantly (I measured 8 seconds for viewer 1, not sure how long it is for viewer 2). ___ 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
Re: [opensource-dev] Review Request: make PREHASH variables char const* const
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/#review196 --- Ship it! I love it! Thanks, this was stopped me from compiling the tests (since some commit not long ago I guess, because I could before). Just one remark. In one test _PREHASH_AgentID is set to AgentID (no doubt a place holder), while in the other you set it to 0 (now needed because it's a const). Perhaps a more symmetric approach is better? I'd opt for setting the other also to a random string, like Grumpity Productengine, or Aleric Inglewood now I think about it. Other options are All Your Base Are Belong To Us and Hi mom!. Got be SURE they aren't used though -- don't want any new 'Grumpity' embarrashments :p - Aleric On Jan. 18, 2011, 7:29 a.m., Boroondas Gupte wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/ --- (Updated Jan. 18, 2011, 7:29 a.m.) Review request for Viewer. Summary --- For the reason for this change, see https://jira.secondlife.com/browse/VWR-24487 and https://jira.secondlife.com/browse/VWR-24522 What I did: In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant pointers to constants by search/replace. Then I tried to compile and added const qualifiers in dependent code as needed to stop the compiler complaining. Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files have been generated from the message template. If this generation wasn't a one-off thing, the generating code must be changed, too, so it won't override this change here when the generation happens the next time. This addresses bug VWR-24487. http://jira.secondlife.com/browse/VWR-24487 Diffs - doc/contributions.txt fc7e5dcf3059 indra/llmessage/message_prehash.h fc7e5dcf3059 indra/llmessage/message_prehash.cpp fc7e5dcf3059 indra/llprimitive/llprimitive.h fc7e5dcf3059 indra/llprimitive/llprimitive.cpp fc7e5dcf3059 indra/llprimitive/llvolumemessage.h fc7e5dcf3059 indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 Diff: http://codereview.secondlife.com/r/100/diff Testing --- Compiled (standalone, 64bit Linux) with and without LL_TESTS. Started the viewer, logged in, walked and flew around a bit. Everything seems to work. Thanks, Boroondas ___ 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
Re: [opensource-dev] Review Request: VWR-24321: Validate textures starting with 00 too.
On Jan. 18, 2011, 12:09 p.m., Merov Linden wrote: indra/newview/lltexturecache.cpp, line 1595 http://codereview.secondlife.com/r/90/diff/1/?file=413#file413line1595 validate_idx being used in a test later, it's not just for (validate_idx == 0) that the behavior will be different. I need to understand better what that idx is all about or you need to give a bit more explanation before I approve this diff. The debug setting CacheValidateCounter is set to 'next_id', which makes clear what it's meaning is: namely, the id that we will check next time. next_idx is a very local variable that is simply set to the value of CacheValidateCounter plus 1, and then that value is stored to CacheValidateCounter again for next time. validate_idx is the ID that is actually being tested this time. Hence, it should be the value of CacheValidateCounter before we increase that. Obviously, those ID's should be in the range 0...255, which is garanteed by doing a modulo 256 on next_id before writing it to CacheValidateCounter. The old code also increases validate_idx *before* it is used. That means that it will be in the range 1...256, and 0 is never tested (note that validate_idx is just increased, there is no modulo applied, so it's obvious that it shouldn't be increased). Even the wording (next_id) suggests that validate_idx should just be the value of CacheValidateCounter, which is the value of the previous next_id... So, after this patch, we get to the following code with validate_idx in the range 0...255, as it should be. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/90/#review191 --- On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/90/ --- (Updated Jan. 14, 2011, 1:02 p.m.) Review request for Viewer. Summary --- Trivial patch, just removes stupidity. This addresses bug VWR-24321. http://jira.secondlife.com/browse/VWR-24321 Diffs - doc/contributions.txt b0bd26c5638a indra/newview/lltexturecache.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/90/diff Testing --- Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: VWR-24311: Uninstall packages that are renewed.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/80/ --- (Updated Jan. 17, 2011, 5:24 a.m.) Review request for Viewer. Changes --- Added requested comments. Summary --- See https://jira.secondlife.com/browse/VWR-24311 Basically, this fixes the TODO comment in install.py but with the difference that we really want to uninstall any old package with the same name, different md5 or not. This addresses bug VWR-24311. http://jira.secondlife.com/browse/VWR-24311 Diffs (updated) - doc/contributions.txt 422f636c3343 scripts/install.py 422f636c3343 Diff: http://codereview.secondlife.com/r/80/diff Testing --- Loads of testing on linux... Installing new packages now cleanly removes the old one first. Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: VWR-24317: Fix of debug warning (printing of unassigned variable)
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/87/
---
(Updated Jan. 17, 2011, 5:32 a.m.)
Review request for Viewer.
Changes
---
Got rid of commented out debug output in replaceSubstitutionStrings. Deleted
the first llwarns and changed a second one into lldebugs.
Summary
---
Fixed a typo that I stumbled upon and added quotes,
and changed the warning to print something that makes
more sense ('replacement' is always empty, since we
didn't find it!)
This addresses bug VWR-24317.
http://jira.secondlife.com/browse/VWR-24317
Diffs (updated)
-
doc/contributions.txt b0bd26c5638a
indra/llui/llnotifications.cpp b0bd26c5638a
Diff: http://codereview.secondlife.com/r/87/diff
Testing
---
Thanks,
Aleric
___
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
Re: [opensource-dev] Review Request: VWR-24520: Don't use pkg_check_modules( ... QUIET ) on CMake 2.8.2
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/97/#review179 --- Ship it! Perfect ;) - Aleric On Jan. 17, 2011, 10:03 a.m., Boroondas Gupte wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/97/ --- (Updated Jan. 17, 2011, 10:03 a.m.) Review request for Viewer. Summary --- Only use QUIET in pkg_check_modules() on CMake =2.8.2 (where it's supported) rather than already on CMake =2.8. This addresses bug VWR-24520. http://jira.secondlife.com/browse/VWR-24520 Diffs - doc/contributions.txt 9e99b2c8fb28 indra/cmake/FindLLQtWebkit.cmake 9e99b2c8fb28 Diff: http://codereview.secondlife.com/r/97/diff Testing --- Configured (standalone) without a .pgk file for libllqtwebkit on Linux with CMake 2.8.1 and CMake 2.8.3. Output as expected. Not tested: * CMake 2.8.2 * system with a .pgk file for libllqtwebkit * non-standalone * Mac, Win Thanks, Boroondas ___ 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
[opensource-dev] Review Request: STORM-864: As as developer, I would like an object oriented wrapper to make safe use of memory pools easier
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/99/ --- Review request for Viewer. Summary --- Please see http://jira.secondlife.com/browse/STORM-864 I made a mercurial repository available for testing here: hg clone https://bitbucket.org/aleric/viewer-development-storm-864 From the commit message: Introduces a LLThreadLocalData class that can be accessed through the static LLThread::tldata(). Currently this object contains two (public) thread-local objects: a LLAPRRootPool and a LLVolatileAPRPool. The first is the general memory pool used by this thread (and this thread alone), while the second is intended for short lived memory allocations (needed for APR). The advantages of not mixing those two is that the latter is used most frequently, and as a result of it's nature can be destroyed and reconstructed on a regular basis. This patch adds LLAPRPool (completely replacing the old one), which is a wrapper around apr_pool_t* and has complete thread-safity checking. Whenever an apr call requires memory for some resource, a memory pool in the form of an LLAPRPool object can be created with the same life-time as this resource; assuring clean up of the memory no sooner, but also not much later than the life-time of the resource that needs the memory. Many, many function calls and constructors had the pool parameter simply removed (it is no longer the concern of the developer, if you don't write code that actually does an libapr call then you are no longer bothered with memory pools at all). However, I kept the notion of short-lived and long-lived allocations alive (see my remark in the jira here: https://jira.secondlife.com/browse/STORM-864?focusedCommentId=235356page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-235356 which requires that the LLAPRFile API needs to allow the user to specify how long they think a file will stay open. By choosing 'short_lived' as default for the constructor that immediately opens a file, the number of instances where this needs to be specified is drastically reduced however (obviously, any automatic LLAPRFile is short lived). This addresses bug STORM-864. http://jira.secondlife.com/browse/STORM-864 Diffs - doc/contributions.txt 422f636c3343 indra/llaudio/llaudioengine_fmod.cpp 422f636c3343 indra/llaudio/llvorbisencode.cpp 422f636c3343 indra/llcharacter/llbvhloader.cpp 422f636c3343 indra/llcharacter/llkeyframemotionparam.cpp 422f636c3343 indra/llcharacter/llstatemachine.cpp 422f636c3343 indra/llcommon/CMakeLists.txt 422f636c3343 indra/llcommon/llapp.cpp 422f636c3343 indra/llcommon/llapr.h 422f636c3343 indra/llcommon/llapr.cpp 422f636c3343 indra/llcommon/llaprpool.h PRE-CREATION indra/llcommon/llaprpool.cpp PRE-CREATION indra/llcommon/llcommon.h 422f636c3343 indra/llcommon/llcommon.cpp 422f636c3343 indra/llcommon/llerror.h 422f636c3343 indra/llcommon/llerror.cpp 422f636c3343 indra/llcommon/llfixedbuffer.cpp 422f636c3343 indra/llcommon/llscopedvolatileaprpool.h PRE-CREATION indra/llcommon/llthread.h 422f636c3343 indra/llcommon/llthread.cpp 422f636c3343 indra/llcommon/llthreadsafequeue.h 422f636c3343 indra/llcommon/llthreadsafequeue.cpp 422f636c3343 indra/llcommon/llworkerthread.h 422f636c3343 indra/llcommon/llworkerthread.cpp 422f636c3343 indra/llcrashlogger/llcrashlogger.cpp 422f636c3343 indra/llimage/llimage.cpp 422f636c3343 indra/llimage/llimagedimensionsinfo.cpp 422f636c3343 indra/llimage/llimagej2c.cpp 422f636c3343 indra/llimage/llimageworker.h 422f636c3343 indra/llimage/llimageworker.cpp 422f636c3343 indra/llmath/llvolumemgr.cpp 422f636c3343 indra/llmessage/llares.cpp 422f636c3343 indra/llmessage/llcurl.cpp 422f636c3343 indra/llmessage/lliohttpserver.h 422f636c3343 indra/llmessage/lliohttpserver.cpp 422f636c3343 indra/llmessage/lliosocket.h 422f636c3343 indra/llmessage/lliosocket.cpp 422f636c3343 indra/llmessage/llmail.h 422f636c3343 indra/llmessage/llmail.cpp 422f636c3343 indra/llmessage/llpumpio.h 422f636c3343 indra/llmessage/llpumpio.cpp 422f636c3343 indra/llmessage/llurlrequest.cpp 422f636c3343 indra/llmessage/message.cpp 422f636c3343 indra/llmessage/tests/networkio.h 422f636c3343 indra/llplugin/llplugininstance.h 422f636c3343 indra/llplugin/llplugininstance.cpp 422f636c3343 indra/llplugin/llpluginmessagepipe.cpp 422f636c3343 indra/llplugin/llpluginprocesschild.cpp 422f636c3343 indra/llplugin/llpluginprocessparent.h 422f636c3343 indra/llplugin/llpluginprocessparent.cpp 422f636c3343 indra/llplugin/llpluginsharedmemory.h 422f636c3343 indra/llplugin/llpluginsharedmemory.cpp 422f636c3343 indra/llplugin/slplugin/slplugin.cpp 422f636c3343 indra/llvfs/lllfsthread.cpp 422f636c3343
[opensource-dev] Review Request: VWR-24519: Spawning of the 'spare' media plugin process makes debugging SLPlugin harder
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/96/ --- Review request for Viewer. Summary --- This patch stops early spawns of spare media plugin processes when the debug setting PluginAttachDebuggerToPlugins is set. The result should be that the terminal with gdb session that pops up when you open a media is the correct one that you intend to debug (instead of an idle one that does nothing; while the process that you want to debug was already spawned an arbitrary amount of time before). This addresses bug VWR-24519. http://jira.secondlife.com/browse/VWR-24519 Diffs - indra/newview/llviewermedia.cpp 422f636c3343 Diff: http://codereview.secondlife.com/r/96/diff Testing --- Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)
On Jan. 14, 2011, 1:31 p.m., Boroondas Gupte wrote: indra/llcommon/lllslconstants.h, line 184 http://codereview.secondlife.com/r/81/diff/1/?file=392#file392line184 Yay for having type modifiers after the core type name. Makes them much easier to understand, especially when there are several cascaded ones. :-) Yeah, I'm strongly convinced that TYPE const is superior in anyway over const TYPE. See http://www.xs4all.nl/~carlo17/cpp/const.qualifier.html for the reasoning. In one line: all type qualifiers work to the left, it's best to PUT them on the left so the whole type is logically uniform in it's construction. The fact that you can legally type 'const TYPE' is just a historically grown misfortune. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/81/#review153 --- On Jan. 16, 2011, 5:53 a.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/81/ --- (Updated Jan. 16, 2011, 5:53 a.m.) Review request for Viewer. Summary --- Turns out that most of my SNOW-800 patch was included in Viewer 2 (albeit without crediting me). However, not everything was used and some more cleaning up was possible. After this patch, and when compiling with optimization, there are no duplicates left anymore that shouldn't be there in the first place: apart from the debug stream iostream guard variable, there are several static variables with the same name (r, r1, r2, etc) but that indeed actually different symbol objects. Then there are a few constant POD arrays that are duplicated a hand full of times because they are accessed with a variable index (so optimizing them away is not possible). I left them like that (although defining those as extern as well would have been more consistent and not slower; in fact it would be faster theoretically because those arrays could share the same cache page then). This addresses bug VWR-24312. http://jira.secondlife.com/browse/VWR-24312 Diffs - doc/contributions.txt 422f636c3343 indra/llcharacter/llanimationstates.cpp 422f636c3343 indra/llcommon/llavatarconstants.h 422f636c3343 indra/llcommon/lllslconstants.h 422f636c3343 indra/llcommon/llmetricperformancetester.h 422f636c3343 indra/llmath/llcamera.h 422f636c3343 indra/llmath/llcamera.cpp 422f636c3343 indra/newview/llviewerobject.cpp 422f636c3343 indra/newview/llvoavatar.cpp 422f636c3343 indra/newview/llvosky.h 422f636c3343 indra/newview/llvosky.cpp 422f636c3343 Diff: http://codereview.secondlife.com/r/81/diff Testing --- Compiled with optimization and then running readelf on the executable to find duplicated symbols. Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: VWR-24317: Incorrect start up warnings: WARNING: remove: Attempting to remove filename: /ramdisk/imprudence/cache/textures/*/*.texture
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/84/ --- (Updated Jan. 16, 2011, 6:12 a.m.) Review request for Viewer. Changes --- Renamed the variable to 'file_maybe_exists', which should cover its meaning more accurately, thanks! Changed the warning into LL_WARNS(TextureCache) Entry has body size of zero but file filename exists. Deleting this file, too. LL_ENDL; The second part is the 'resolution' of the problem. Your proposal sounded a bit too much as if the warning was about the fact that the file was being removed... Summary --- This turned out to be a simple matter of trying to remove non-existant files: A texture with a 'body size' of 0 doesn't have a texture body file. This addresses bug VWR-24317. http://jira.secondlife.com/browse/VWR-24317 Diffs (updated) - indra/newview/lltexturecache.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/84/diff Testing --- Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: VWR-24317: Incorrect start up warnings: WARNING: ll_apr_warn_status: APR: No such file or directory
On Jan. 14, 2011, 1:47 p.m., Boroondas Gupte wrote: indra/newview/llappviewer.cpp, lines 3091-3094 http://codereview.secondlife.com/r/83/diff/1/?file=402#file402line3091 what's the reason for moving the LL_INFOS around? The last two, in order to print the correct value that gLastExecEvent is being set to: depending on the conditional, the value is set to what was printed, or to another value. The first hunk to have more symmetric code and treat that part the same as the others: first set the variable and then print it's contents. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/83/#review154 --- On Jan. 14, 2011, 12:48 p.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/83/ --- (Updated Jan. 14, 2011, 12:48 p.m.) Review request for Viewer. Summary --- At start up one can get the following “warnings”: 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or directory 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: /home/aleric/.imprudence/logs/Imprudence.logout_marker 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or directory 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: /home/aleric/.imprudence/logs/Imprudence.llerror_marker 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or directory 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: /home/aleric/.imprudence/logs/Imprudence.error_marker This is nonsense since it is perfectly ok when those files don’t exist. This addresses bug VWR-24317. http://jira.secondlife.com/browse/VWR-24317 Diffs - indra/newview/llappviewer.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/83/diff Testing --- Thanks, Aleric ___ 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
Re: [opensource-dev] Link times
I'm afraid that VWR-24366 won't reduce link times significantly. On Fri, Jan 14, 2011 at 11:49 PM, Boroondas Gupte slli...@boroon.dasgupta.ch wrote: On 01/14/2011 06:04 PM, Jonathan Welch wrote: I just did a quick study on link times for various viewers on my 2Gb XP system Viewer 1st link 2nd link CV 1.220:53 0:24 SG 1.5 3:30 2:07 V2.5 6:18 6:01 The long time for 2.5 be due to VWR-24366. Dunno why linking Cool Viewer (that's what CV stands for, isn't it?) is so much quicker than SG 1.5, though. Good news: A fix is being reviewed at https://codereview.secondlife.com/r/95/ Cheers, Boroondas ___ 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 ___ 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
[opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/81/ --- Review request for Viewer. Summary --- Turns out that most of my SNOW-800 patch was included in Viewer 2 (albeit without crediting me). However, not everything was used and some more cleaning up was possible. After this patch, and when compiling with optimization, there are no duplicates left anymore that shouldn't be there in the first place: apart from the debug stream iostream guard variable, there are several static variables with the same name (r, r1, r2, etc) but that indeed actually different symbol objects. Then there are a few constant POD arrays that are duplicated a hand full of times because they are accessed with a variable index (so optimizing them away is not possible). I left them like that (although defining those as extern as well would have been more consistent and not slower; in fact it would be faster theoretically because those arrays could share the same cache page then). This addresses bug VWR-24312. http://jira.secondlife.com/browse/VWR-24312 Diffs - doc/contributions.txt b0bd26c5638a indra/llcharacter/llanimationstates.cpp b0bd26c5638a indra/llcommon/llavatarconstants.h b0bd26c5638a indra/llcommon/lllslconstants.h b0bd26c5638a indra/llcommon/llmetricperformancetester.h b0bd26c5638a indra/llmath/llcamera.h b0bd26c5638a indra/llmath/llcamera.cpp b0bd26c5638a indra/newview/llviewerobject.cpp b0bd26c5638a indra/newview/llvoavatar.cpp b0bd26c5638a indra/newview/llvosky.h b0bd26c5638a indra/newview/llvosky.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/81/diff Testing --- Compiled with optimization and then running readelf on the executable to find duplicated symbols. Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24315: SNOW-796: Clicking 'Reset to default' in the Debug Settings floater doesn't update cached control values.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/82/ --- Review request for Viewer. Summary --- See https://jira.secondlife.com/browse/VWR-24315 This addresses bug VWR-24315. http://jira.secondlife.com/browse/VWR-24315 Diffs - doc/contributions.txt b0bd26c5638a indra/newview/llfloatersettingsdebug.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/82/diff Testing --- I have some other patch that allows me to move my avatar up and down vertically (by faking it has a different height). When I open the Debug Settings floater and change the value with the up/down arrows, the avatar is immediately updated (changes height), but resetting the value (to zero) had no effect. With this patch, resetting caused the avatar to immediately change it's height to an Z-offset of zero again, as it should. Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24317: Incorrect start up warnings: WARNING: ll_apr_warn_status: APR: No such file or directory
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/83/ --- Review request for Viewer. Summary --- At start up one can get the following “warnings”: 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or directory 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: /home/aleric/.imprudence/logs/Imprudence.logout_marker 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or directory 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: /home/aleric/.imprudence/logs/Imprudence.llerror_marker 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or directory 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: /home/aleric/.imprudence/logs/Imprudence.error_marker This is nonsense since it is perfectly ok when those files don’t exist. This addresses bug VWR-24317. http://jira.secondlife.com/browse/VWR-24317 Diffs - indra/newview/llappviewer.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/83/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24317: Incorrect start up warnings: WARNING: remove: Attempting to remove filename: /ramdisk/imprudence/cache/textures/*/*.texture
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/84/ --- Review request for Viewer. Summary --- This turned out to be a simple matter of trying to remove non-existant files: A texture with a 'body size' of 0 doesn't have a texture body file. This addresses bug VWR-24317. http://jira.secondlife.com/browse/VWR-24317 Diffs - indra/newview/lltexturecache.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/84/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24317: Incorrect start up warnings: WARNING: addFeature: LLFeatureList::Attempting to add preexisting feature Disregard128DefaultDrawDistance
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/85/ --- Review request for Viewer. Summary --- Fixes a bug that causes the last line of the feature table file to be read twice. This patch also removes the check for name.empty() because that will never be true, so might as well remove it. This addresses bug VWR-24317. http://jira.secondlife.com/browse/VWR-24317 Diffs - indra/newview/llfeaturemanager.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/85/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24317: Incorrect start up warnings: WARNING: isFeatureAvailable: Feature RenderCubeMap not on feature list!
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/86/ --- Review request for Viewer. Summary --- Fixes this warning. Note that LLCubeMap::sUseCubeMaps is set to true by default, so this patch only has effect when the feature is actually NOT available. I tested that LLCubeMap::sUseCubeMaps is NOT used before the point where it is initialized now. This addresses bug VWR-24317. http://jira.secondlife.com/browse/VWR-24317 Diffs - indra/newview/llappviewer.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/86/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24319: Fix the errors QCursor: Cannot create bitmap cursor; invalid bitmap(s) at start up of the webkit plugin, on linux.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/88/ --- Review request for Viewer. Summary --- These are caused because builtin resources of QtWebKit weren't initialized. This patch adds this initialization. This addresses bug VWR-24319. http://jira.secondlife.com/browse/VWR-24319 Diffs - doc/contributions.txt b0bd26c5638a indra/media_plugins/webkit/media_plugin_webkit.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/88/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24320: Don't dump callstacks at clean exit of viewer.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/89/ --- Review request for Viewer. Summary --- See http://jira.secondlife.com/browse/VWR-24320 This addresses bug VWR-24320. http://jira.secondlife.com/browse/VWR-24320 Diffs - doc/contributions.txt b0bd26c5638a indra/newview/llappviewer.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/89/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24321: Validate textures starting with 00 too.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/90/ --- Review request for Viewer. Summary --- Trivial patch, just removes stupidity. This addresses bug VWR-24321. http://jira.secondlife.com/browse/VWR-24321 Diffs - doc/contributions.txt b0bd26c5638a indra/newview/lltexturecache.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/90/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24333: Hardening against use of getLindenUserDir() before logging in.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/91/ --- Review request for Viewer. Summary --- Without this patch, getLindenUserDir() is sometimes used without checking if it returns an empty value, resulting in trying to open an empty file and then gracefully recovering from that. So, this patch doesn't really fix a bug. However it might prevent one in the future. Note that this DID lead to a bug in Viewer 1 code, so it's possible. The main threat is that some singleton class that uses getLindenUserDir (indirectly) is instantiated before logging in: A singleton is only created once and when it is initialized with an empty getLindenUserDir() then that can remain. This patch aborts when the viewer is compiled in debug mode (not in release mode, when it will do what it did before this patch) and when getLindenUserDir() is called before it was initialized, unless the developer explicitely passes 'true' (empty_ok) to getLindenUserDir, signaling that they considered the possibility that the function is being called before the user logged in. This addresses bug VWR-24333. http://jira.secondlife.com/browse/VWR-24333 Diffs - indra/llvfs/lldir.h 422f636c3343 indra/llvfs/lldir.cpp 422f636c3343 indra/newview/llappviewer.cpp 422f636c3343 indra/newview/llbottomtray.cpp 422f636c3343 indra/newview/llfilepicker.cpp 422f636c3343 indra/newview/llnavigationbar.cpp 422f636c3343 indra/newview/llsearchhistory.h 422f636c3343 indra/newview/llurlhistory.cpp 422f636c3343 indra/newview/llviewerinventory.cpp 422f636c3343 indra/newview/llviewermedia.cpp 422f636c3343 indra/newview/llvoiceclient.cpp 422f636c3343 Diff: http://codereview.secondlife.com/r/91/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24334: Add support for PluginAttachDebuggerToPlugins to linux
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/92/ --- Review request for Viewer. Summary --- Added: On linux, when PluginAttachDebuggerToPlugins is set to TRUE, execute the value of the environment variable LL_DEBUG_TERMINAL_COMMAND where a (obligatory) %s is replaced with the contents of the value of the environment variable LL_DEBUG_GDB_PATH followed by the string -n /proc//exe , where is the integer of the SLPlugin process we wish to attach to. The default value of LL_DEBUG_TERMINAL_COMMAND, if no environment variable is found, is /usr/bin/xterm -geometry 160x24+0+0 -e %s and the default value of LL_DEBUG_GDB_PATH is /usr/bin/gdb. As such, it is possible to use a different terminal, for example setting the enviroment variable: LL_DEBUG_TERMINAL_COMMAND=/usr/bin/gnome-terminal --geometry=165x24-0+0 -e %s And you can pass other parameters to gdb (and/or change it's path), for example by setting the enviroment variable: LL_DEBUG_GDB_PATH=/usr/local/bin/gdb -x /home/aleric/mygdbinit which would open a different gdb and start with executing commands from /home/aleric/mygdbinit (no other .gdbinit is read). This addresses bug VWR-24334. http://jira.secondlife.com/browse/VWR-24334 Diffs - doc/contributions.txt b0bd26c5638a indra/llcommon/llprocesslauncher.h b0bd26c5638a indra/llplugin/llpluginprocessparent.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/92/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24337: Possible crash on llassert_always(purge_list.size() = entries_to_purge)
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/93/ --- Review request for Viewer. Summary --- Just fixed the logic, so entries_to_purge won't become negative anymore, and the rest. This addresses bug VWR-24337. http://jira.secondlife.com/browse/VWR-24337 Diffs - doc/contributions.txt b0bd26c5638a indra/newview/lltexturecache.cpp b0bd26c5638a Diff: http://codereview.secondlife.com/r/93/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24354: Fix manifest dependencies for linux
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/94/ --- Review request for Viewer. Summary --- See http://jira.secondlife.com/browse/VWR-24354 This addresses bug VWR-24354. http://jira.secondlife.com/browse/VWR-24354 Diffs - doc/contributions.txt b0bd26c5638a indra/newview/CMakeLists.txt b0bd26c5638a Diff: http://codereview.secondlife.com/r/94/diff Testing --- Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24366: CMAKE_EXE_LINKER_FLAGS not honored when linking the viewer binary if -DLL_TESTS:BOOL=ON
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/95/ --- Review request for Viewer. Summary --- Setting CMAKE_EXE_LINKER_FLAGS to because the tests need that is pretty hard measure. Not only is it not necessary to do so, it also changes how the viewer is linked depending on a whether or not the tests are compiled and that is not good. The reason that this was needed is that libgmock is underlinked (see http://wiki.mandriva.com/en/Underlinking), which is not compatible with -Wl,--as-needed that is being used on linux. libgmock.so.0 needs a symbol that is defined in libgtest.so.o, but -lgtest was not passed to the linker when creating libgmock.so.0: Underlinked (no libgtest.so.o): $ objdump -p /usr/lib/libgmock.so.0 | grep NEEDED NEEDED libstdc++.so.6 NEEDED libm.so.6 NEEDED libc.so.6 NEEDED libgcc_s.so.1 The solution is to wrap between -Wl,--no-as-needed -lgtest -Wl,--as-needed causing it to be added again. This is only needed on linux, since that the only platform that we use -Wl,--as-needed on. Moreover, we can just set GOOGLEMOCK_LIBRARIES to gmock -Wl,--no-as-needed gtest -Wl,--as-needed since that is only passed to TARGET_LINK_LIBRARIES which only adds -l in front of 'things' that don't start with '-', to allow you do pass special flags like this. This addresses bug VWR-24366. http://jira.secondlife.com/browse/VWR-24366 Diffs - doc/contributions.txt 422f636c3343 indra/cmake/GoogleMock.cmake 422f636c3343 indra/cmake/LLAddBuildTest.cmake 422f636c3343 Diff: http://codereview.secondlife.com/r/95/diff Testing --- Thanks, Aleric ___ 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
Re: [opensource-dev] Why no single axis resize on linked prims??
And now for the real answer... You can't resize a prim along an arbitrary axis, so if any linked prim is rotated, it's simply not possible (in 99.9% of the cases). You might be able to pull it of for many special cases by changing sheer values, and use rotation (and texture rotation to correct for that) etc, but even then it only works in so many cases, never along any arbitrary axis for any arbirary prim shape. On Sun, Jan 9, 2011 at 6:40 PM, Robert Martin robertl...@gmail.com wrote: Does anybody know of a good reason why when you are resizing a set of linked prims you do not have the single axis resize?? (also could we get a way to resize a linkset with 0.001 meters as a floor not as a locking measurement?) -- Robert L Martin ___ 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 ___ 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
Re: [opensource-dev] Review Request: STORM-830 Volume slider isn't properly remembered if set to zero
On Jan. 6, 2011, 5:37 p.m., Aleric Inglewood wrote: This is really not how you want to deal with this bug :/. It's a known fact that audio mixers are very bad with low volumes. Setting a volume to 0 (or something really small) can put a very high load on the CPU for the audio mixer, which causes severe problems. See https://jira.secondlife.com/browse/VWR-14914 So, on the contrary (as VWR-14914 fixed a horrible bug that made FPS drop drastically): when the volume is set to 0 (or even close to zero) the audio channel has to be muted and not mixed, ever. Assuming that VWR-14914 is in Viewer 2, and wasn't broken in the meantime, a volume of 0 would cause the channel to be muted, but setting it to 0.01 will not cause it to be muted, but result in a high CPU load. After discussion on IRC Jonathan explained to me what this is all about. The patch is correct. Nevertheless, I was confused about the fact that this value of 0.01 is never going to be USED. Perhaps a different value and comment can avoid that in the future when other coders look at it. mInternalGain is only ever compared with, and the whole point of this patch is to make sure that the first comparison fails (I now understand). A common value used to make sure that a first comparison fails is a value outside the valid range of that variable. The valid range being [0, 1], I'd suggest using -1 and adding a comment in the lines of: // Make sure that the first call to setMasterGain will cause setInternalGain to be called. mInternalGain = -1.f; - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/72/#review128 --- On Jan. 6, 2011, 2:37 p.m., Jonathan Yap wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/72/ --- (Updated Jan. 6, 2011, 2:37 p.m.) Review request for Viewer. Summary --- There is an edge case in setMasterGain during startup which prevents setInternalGain from being called if the master volume setting and mInternalGain both equal 0. Setting mInternalGain to a very low but non-zero value fixes this issue. This addresses bug STORM-830. http://jira.secondlife.com/browse/STORM-830 Diffs - indra/llaudio/llaudioengine.cpp 6d44f0d85a80 Diff: http://codereview.secondlife.com/r/72/diff Testing --- In Preferences / Sound Media tested: Buttons Ambient Sound Effects Stream Music Media Voice Chat Thanks, Jonathan ___ 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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
On Jan. 7, 2011, 9:36 a.m., Joshua Linden wrote: I believe Aleric's comment is accurate. Logic testing for a prefix should be removed from the patch, and the flag should simply always be specified in this case. It is notable that the flag does trigger exactly the same test that is present in the patch (i.e. it's not case sensitive, it replicates prefix testing in several other places in the code base, etc). A more general fix might be to refactor all of the places that do prefix testing, but that wouldn't affect this specific issue. Again, the patch should be reduced to one line that simply adds the desired flag. Jonathan Yap wrote: Joshua, if the check for /me and /me' were not present and CHAT_STYLE_IRC was always passed in then all llInstantMessages from objects would be treated as if /me had been sent. I don't think this is what is desired. @Joshua : Um yes - all of that seemed logical. But the existing code is far from logical, or clean. I just had a discussion with Jonathan on IRC and now understand that the meaning of the CHAT_STYLE_IRC is actually we found this message to start with /me, please BLINDLY replace the first 3 characters with the name of the object/avatar. The name of the flag is horribly wrong imho. Also, then I suggested to change the code in the what I had assumed it was: set the flag whenever replacement is necessary and just leave it to the replacement code to check for it. That would therefore require an additional change: make the code that now only tests if the flag is set ALSO test if the message indeed starts with /me or /me' (and having gotten rid of the code duplication, it then could easily be extended in the future). Unfortunately, not only the testing for /me is scattered all over the place, also the code that does the actual replacement of the first 3 characters exists in many places! ... So, without making this a much larger project, I suppose adding one more duplication of code that checks for /me isn't the worst of things, and at least it is an immediate fix for the problem at hand :/. I have no objections anymore if the patch would be used as-is. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/71/#review132 --- On Jan. 5, 2011, 6:14 p.m., Jonathan Yap wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/71/ --- (Updated Jan. 5, 2011, 6:14 p.m.) Review request for Viewer. Summary --- The /me in the lsl code below would be displayed rather than being translated to a name: llInstantMessage(llGetOwner(),/me Hello, Avatar!); This addresses bug STORM-829. http://jira.secondlife.com/browse/STORM-829 Diffs - indra/newview/llviewermessage.cpp 845cab866155 Diff: http://codereview.secondlife.com/r/71/diff Testing --- Thanks, Jonathan ___ 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
Re: [opensource-dev] Review Request: STORM-826 (partial): fix line endings in files that use a mix of CRLF and LF
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/70/#review119 --- You missed indra/newview/llfloaterwebcontent.cpp Also, why not fix the only .xml with carriage returns in it: indra/newview/skins/default/xui/en/floater_web_content.xml Configuration files with carriage returns: indra/cmake/GetPrerequisites_2_8.cmake indra/cmake/LLAddBuildTest.cmake - Aleric On Jan. 5, 2011, 7:19 a.m., Oz Linden wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/70/ --- (Updated Jan. 5, 2011, 7:19 a.m.) Review request for Viewer. Summary --- This is a simple change to correct existing line endings - I scanned all of viewer-development to identify files that had a mixture of CRLF and LF endings and converted them to just LF. What to do about preventing future such will be dealt with separately... This addresses bug storm-826. http://jira.secondlife.com/browse/storm-826 Diffs - indra/newview/llfloaterwebcontent.h 845cab866155 indra/newview/llimview.h 845cab866155 indra/newview/llimview.cpp 845cab866155 indra/newview/lllogchat.cpp 845cab866155 Diff: http://codereview.secondlife.com/r/70/diff Testing --- Thanks, Oz ___ 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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/71/#review120 --- What about /Me, /ME or /me followed by another punctuation? Ie, /me?, /me!, etc... Just asking because these comparisions with just /me and /me' seem very limited, almost weird. More logical would be to not check anything at ALL - and either expand things, or not. What happens if you just set a flag saying whatever is in this string, don't expand /me, /who, /whois, /kick etc without at that point checking for one specific case (missing possibly many other variations). - Aleric On Jan. 5, 2011, 6:14 p.m., Jonathan Yap wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/71/ --- (Updated Jan. 5, 2011, 6:14 p.m.) Review request for Viewer. Summary --- The /me in the lsl code below would be displayed rather than being translated to a name: llInstantMessage(llGetOwner(),/me Hello, Avatar!); This addresses bug STORM-829. http://jira.secondlife.com/browse/STORM-829 Diffs - indra/newview/llviewermessage.cpp 845cab866155 Diff: http://codereview.secondlife.com/r/71/diff Testing --- Thanks, Jonathan ___ 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
Re: [opensource-dev] Review Request: STORM-826 (partial): fix line endings in files that use a mix of CRLF and LF
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/70/#review124 --- Ship it! Should be ok, I see no diff on the review board at all :p I didn't actually apply the diff and check if all carriage returns are gone now, but I don't think that's the idea of the review board, is it? - Aleric On Jan. 6, 2011, 10:20 a.m., Oz Linden wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/70/ --- (Updated Jan. 6, 2011, 10:20 a.m.) Review request for Viewer. Summary --- This is a simple change to correct existing line endings - I scanned all of viewer-development to identify files that had a mixture of CRLF and LF endings and converted them to just LF. The diff apparently won't show the change in line ending characters see the repo (in the issue) for the real change. I expanded the scope of this to also convert some files that were all the same CRLF endings but did not obviously need to be. I left files that were clearly windows specific alone on the theory that non-windows users probably won't need to touch them, and the windows tools might care. This addresses bug storm-826. http://jira.secondlife.com/browse/storm-826 Diffs - indra/cmake/GetPrerequisites_2_8.cmake 6d44f0d85a80 indra/cmake/LLAddBuildTest.cmake 6d44f0d85a80 indra/newview/llfloaterwebcontent.h 6d44f0d85a80 indra/newview/llfloaterwebcontent.cpp 6d44f0d85a80 indra/newview/llimview.h 6d44f0d85a80 indra/newview/llimview.cpp 6d44f0d85a80 indra/newview/lllogchat.cpp 6d44f0d85a80 indra/newview/tests/llremoteparcelrequest_test.cpp 6d44f0d85a80 indra/viewer_components/updater/tests/llupdaterservice_test.cpp 6d44f0d85a80 Diff: http://codereview.secondlife.com/r/70/diff Testing --- Thanks, Oz ___ 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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
On Jan. 6, 2011, 7 a.m., Aleric Inglewood wrote:
What about /Me, /ME or /me followed by another punctuation? Ie, /me?,
/me!, etc...
Just asking because these comparisions with just /me and /me' seem
very limited,
almost weird. More logical would be to not check anything at ALL - and
either expand
things, or not. What happens if you just set a flag saying whatever is in
this
string, don't expand /me, /who, /whois, /kick etc without at that point
checking
for one specific case (missing possibly many other variations).
Jonathan Yap wrote:
If it was me writing the original code I would not have made it
case-sensitive, but as this is a bug fix and not a new feature I am following
the current design of having just /me work.
I didn't suggest to make it case insensitive, I wondered what happens
when you use /ME instead of /me with and without the patch.
And I wonder why it is necessary at all to compare a string with /me .
At the very least that indicates code duplication.
Let me clarify,
void do_it(std::string const str)
{
if (!flag str == /me)
...
else
...
}
Bad code:
if (str == /me)
flag = 1;
do_it(str);
---
Code that makes more sense:
flag = 1;
do_it(str);
But keep in mind that I didn't look at the actual code ;). I just looked at
your patch.
- Aleric
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/71/#review120
---
On Jan. 5, 2011, 6:14 p.m., Jonathan Yap wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/71/
---
(Updated Jan. 5, 2011, 6:14 p.m.)
Review request for Viewer.
Summary
---
The /me in the lsl code below would be displayed rather than being
translated to a name:
llInstantMessage(llGetOwner(),/me Hello, Avatar!);
This addresses bug STORM-829.
http://jira.secondlife.com/browse/STORM-829
Diffs
-
indra/newview/llviewermessage.cpp 845cab866155
Diff: http://codereview.secondlife.com/r/71/diff
Testing
---
Thanks,
Jonathan
___
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
Re: [opensource-dev] Review Request: VWR-24100 Settings.xml: redundant entries and unnecessary tag
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/18/#review107 --- Ship it! - Aleric On Dec. 23, 2010, 12:12 p.m., Jonathan Yap wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/18/ --- (Updated Dec. 23, 2010, 12:12 p.m.) Review request for Viewer. Summary --- I wrote two programs that use settings.xml to produce this massive table: http://wiki.secondlife.com/wiki/Debug_Settings While doing this I found 4 places with duplicate entries and 1 entry that is repeated 4 times. There is also a pair of unnecessary tags. Having these cleaned out would make running my program, and thus updating the wiki table, easier. I've erased all but the last entry for these redundant debug settings. This addresses bug vwr-24100. http://jira.secondlife.com/browse/vwr-24100 Diffs - indra/newview/app_settings/settings.xml 46a990f8296f Diff: http://codereview.secondlife.com/r/18/diff Testing --- Thanks, Jonathan ___ 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
Re: [opensource-dev] Debugging Snowstorm under Linux x64 with GDB locks the whole X session
On Wed, Dec 29, 2010 at 10:05 PM, Nicky D. sl.nicky...@googlemail.com wrote: But I just *run* snowstorm under gdb by setting LL_WRAPPER. I have not one breakpoint set, neither do I interrupt the program at all. I just use the viewer and it will just lock up the whole X display sooner or later. When I use the viewer without gdb all is fine, no crashes, hang, all good. That is not normal... My X display did lock up too.. until I replaced my videocard ;) If you just let it run, then there is no reason for it to hang X imho :/. But if it only happens when you run gdb and not otherwise... I don't know. Sounds like a software problem then though. ___ 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
Re: [opensource-dev] Review Request: Reenable the LLMatrix3::orthogonalize test in llmath/tests/m3math_test.cpp
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/67/#review101 --- I think this line should be deleted, not commented out. If there still is a reason to think that it might need to be uncommented later than apparently we aren't sure the test really works, in which case I think it should be skipped until we are certain and this is fixed on all platforms. - Aleric On 2010-12-28 10:05:04, Wolfpup Lowenhar wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/67/ --- (Updated 2010-12-28 10:05:04) Review request for Viewer. Summary --- This patch is to fix this test is working in Windows environments This addresses bug https://jira.secondlife.com/browse/VWR-24332. http://jira.secondlife.com/browse/https://jira.secondlife.com/browse/VWR-24332 Diffs - indra/llmath/tests/m3math_test.cpp 940cd25d4b78 Diff: http://codereview.secondlife.com/r/67/diff Testing --- Re-enabled the test and built the viewer including all tests and have no errors and all of the test done in m3math_test are reporting that they all succeed. Thanks, Wolfpup ___ 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
Re: [opensource-dev] Review Request: STORM-737 Add + menu to Inventory/Recent
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/65/#review88 --- indra/newview/llpanelmaininventory.cpp http://codereview.secondlife.com/r/65/#comment62 While you're removing that empty line anyway, I thought I'd help you to not learn the bad coding habbits of whoever wrote the original code ;). operator== returns a bool, might as well do the conversions one step later (if at all) and use a bool here. Using 'bool' always has the preference (BOOL is ugly window-ism). Certainly in this case since !recent_active converts it to a bool again! (bool -- BOOL - bool - BOOL now). Secondly, when testing if a variable is equal to a literal/constant, I think that's better readable to put the variable up front, thus: mActivePanel()-getName() == Recent Items. Finally, although you may choose to leave it like it is, be aware that the extra 'variable' recent_active here was only added as pseudo 'comment' and to because the monitor of the original coder wasn't wide enough. If you have a normal 22 inch like all devs, you might also consider the more professional looking: // New Folder is broken for the Recent Items tab. Do not enable it for that case. mMenuAdd-getChildLLMenuItemGL(New Folder)-setEnabled(mActivePanel-getName() != Recent Items); - Aleric On 2010-12-23 13:25:31, Jonathan Yap wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/65/ --- (Updated 2010-12-23 13:25:31) Review request for Viewer. Summary --- This change enables the + menu in Inventory/Recent It grays out New Folder in this menu It enables identical menu entries when you right click on an inventory item. Question: Is graying out New Folder best done where I am doing it now -- in llpanelmaininventory.cpp / LLPanelMainInventory::onAddButtonClick() This addresses bug storm-737. http://jira.secondlife.com/browse/storm-737 Diffs - doc/contributions.txt e843e274fa58 indra/newview/llinventorybridge.cpp e843e274fa58 indra/newview/llpanelmaininventory.cpp e843e274fa58 Diff: http://codereview.secondlife.com/r/65/diff Testing --- I opened up Inventory/My Inventory and used all the New xxx options for both right clicking on an inventory item and also from the + menu. I then changed to the Recent tab and performed the same steps. New items were created as expected, except New Folder was not an option via either method when the Recent tab was active. Thanks, Jonathan ___ 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
Re: [opensource-dev] Review Request: STORM-807 update returnability of objects based on new encroachment rules
The attachment to this post, of 2.5 MB, caused my firewall PC to overload while trying to determine if it is spam, spamd to time out and temporarily reject the mail, which caused google to try to deliver it over and over again (27 times so far). I'd like to request that large attachments are not attached, but instead just provide a link to them (upload them to the jira, say). On Wed, Dec 22, 2010 at 2:49 AM, Andrew Meadows and...@lindenlab.com wrote: I've added a feature to the SL simulator that allows encroaching (*) objects to be returned. The ETA for this is sometime in early 2011 (probably January) and I'll be adding some wiki documentation about the returnability of encroaching objects soon. In the meantime I've added viewer-side UI support for returning encroaching objects. It should not hurt the viewer to add these changes before the server stuff is deployed. Here is the merge-request jira: https://jira.secondlife.com/browse/STORM-807 The changes can be pulled from here: http://bitbucket.org/andrew_linden/viewer-development The diff is attached. - Andrew (*) Encroaching objects are positioned on one person's land parcel but nevertheless extend part of themselves into a neighbor's parcel. ___ 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 ___ 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
Re: [opensource-dev] Review Request: KDU Improvements: add unit tests for llkdu
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/63/#review78 --- Huh - I wrote this a long time ago (before the others commented)... apparently I forgot to click on Publish Review! indra/llkdu/llimagej2ckdu.h http://codereview.secondlife.com/r/63/#comment45 This feels wrong. Those functions are implementations of the base class interface, they are called by the base class: if they need to be called directly, something is wrong. If this is made public because the unit test needs access, then instead add a friend for the unit test class. - Aleric On 2010-12-22 23:51:18, Merov Linden wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/63/ --- (Updated 2010-12-22 23:51:18) Review request for Viewer. Summary --- Unit tests addition: - add tests for llkdu - turned back on and fix unit tests for llimage - turned back on and fix unit tests for llworldmap and llworldmipmap This addresses bug STORM-744. http://jira.secondlife.com/browse/STORM-744 Diffs - indra/llimage/CMakeLists.txt 5d69e36a53ee indra/llimage/tests/llimageworker_test.cpp 5d69e36a53ee indra/llkdu/CMakeLists.txt 5d69e36a53ee indra/llkdu/llimagej2ckdu.h 5d69e36a53ee indra/llkdu/tests/llimagej2ckdu_test.cpp PRE-CREATION indra/newview/CMakeLists.txt 5d69e36a53ee indra/newview/tests/llworldmap_test.cpp 5d69e36a53ee indra/newview/tests/llworldmipmap_test.cpp 5d69e36a53ee Diff: http://codereview.secondlife.com/r/63/diff Testing --- Thanks, Merov ___ 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
Re: [opensource-dev] Review Request: KDU Improvements: add unit tests for llkdu
On 2010-12-23 17:29:10, Aleric Inglewood wrote: indra/llkdu/llimagej2ckdu.h, line 58 http://codereview.secondlife.com/r/63/diff/1/?file=255#file255line58 This feels wrong. Those functions are implementations of the base class interface, they are called by the base class: if they need to be called directly, something is wrong. If this is made public because the unit test needs access, then instead add a friend for the unit test class. After reading the other comments, I had to add that accessing them by inheritance would definitely be more clean. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/63/#review78 --- On 2010-12-22 23:51:18, Merov Linden wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/63/ --- (Updated 2010-12-22 23:51:18) Review request for Viewer. Summary --- Unit tests addition: - add tests for llkdu - turned back on and fix unit tests for llimage - turned back on and fix unit tests for llworldmap and llworldmipmap This addresses bug STORM-744. http://jira.secondlife.com/browse/STORM-744 Diffs - indra/llimage/CMakeLists.txt 5d69e36a53ee indra/llimage/tests/llimageworker_test.cpp 5d69e36a53ee indra/llkdu/CMakeLists.txt 5d69e36a53ee indra/llkdu/llimagej2ckdu.h 5d69e36a53ee indra/llkdu/tests/llimagej2ckdu_test.cpp PRE-CREATION indra/newview/CMakeLists.txt 5d69e36a53ee indra/newview/tests/llworldmap_test.cpp 5d69e36a53ee indra/newview/tests/llworldmipmap_test.cpp 5d69e36a53ee Diff: http://codereview.secondlife.com/r/63/diff Testing --- Thanks, Merov ___ 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
Re: [opensource-dev] Review Request: STORM-702 Make it possible to wear partial outfits
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
Re: [opensource-dev] Review Request: Update returnability of objects based on new encroachment rules
I have no problems with the intent of returning encroaching objects, but I do with the way encroaching is detected: You should not add this feature when it is in a broken state, and I think it is broken when it will think that objects are encroaching when not even the minimal aligned bounding box of the object is encroaching. I'm not speaking about a mostly transparent tree even, or a L-shaped object at the corner of a parcel. But even, just the simplest and most OBVIOUS ways to drastically decrease the size of mega prims - or just the using cut path on a box. That should be detected - as should the minimal bb for sculpties be more accurately determined imho. On Wed, Dec 22, 2010 at 8:51 PM, Andrew Meadows and...@lindenlab.com wrote: A note about the returnable encroachment feature... It introduces a new definition I'm calling Estate Content. Estate content is stuff owned by the Estate Owner, or an Estate Manager. It is treated differently by the returnable encroachment feature, namely in order for Estate Content to be returned there is a special per-region setting that mus be enabled. That is, each region will have two independent settings: allow_return_encroaching_object = true/false allow_return_encroaching_estate_object = true/false The reason for this split is that some estates already use encroachment as a feature, where there are some encroaching content that is considered public content such as roads and communal buildings. As to encroaching objects on the mainland... We'll eventually enable return of encroaching objects on the mainland after some private estates have been using it for a while. I expect we'll enable it on a few mainland regions and see how things go, then expand. Some mainland regions use encroachment as a feature and I think we should move a little slowly there until we have a good understanding of how things should be configured for the least damage. After that I suspect we'll enable encroachment return across all of mainland. As to cross-region encroachment, where an object is on one region but sticks into the parcel of another region... Return of cross-region encroaching objects is not supported yet, but we plan to implement it before mesh ships (early 2011). This means simscape megaprims will be at risk for encroachment return via neighboring regions. Estate owners can always disable encroachment return if they prefer to protect their simscapes, or they can just make sure their simscapes fall into the Estate Content category. Meanwhile on the mainland I think the returnable encroachment outweighs the simscape megaprim hack so I will push to make the demise of this hack a non-blocker. Eventually I'm sure we can come up with a solution and I hope it is a real simscape feature that doesn't rely on really really big megaprims. - Andrew On 12/22/2010 10:57 AM, Liny Odell wrote: Another key point is simscapes that use megaprims that obviously covers all parcels but is only visible along the outside of the region along the void. Also, what about prims that are centered in a neiboring region? Are thous detected as encroaching? On Wed, Dec 22, 2010 at 10:45 AM, Aleric Inglewood aleric.inglew...@gmail.com mailto:aleric.inglew...@gmail.com wrote: This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/56/ I have the feeling that this notion of'encroachesOwned' is going to cause a lot of trouble in the case of sculptures. At the very least you should use use an aligned bb that is the minimal bb for anything *visible* of a prim. In the case of the sculptures this requires a special function to find the min/max coordinates of all the points used. The same story holds for path cut-off (mega) prims. Ie, if I resize a megaprim (the ONLY way to resize a mega prim is by using one of those prim torture tricks) then it's very possible one ends up with a prim that is just 1/3 of it'ssize. It wouldn't be fair when a house that clearly ends on ones own parcel isdetected to encroach another parcel, just because the viewer code is too lame to take path cutting and sphere dimples etc into account. - Aleric On December 22nd, 2010, 9:37 a.m., Merov Linden wrote: Review request for Viewer and Andrew Meadows. By Merov Linden. /Updated 2010-12-22 09:37:08/ Description The object-vs-parcel overlap test is done by building axis-aligned bounding boxes (AABB) about each prim of the selected objects and then checking for overlap between those boxes and self- and group-owned parcels. *Bugs: * STORM-807 http://jira.secondlife.com/browse/STORM-807 Diffs * indra/llcommon/llversionserver.h (fc82190a3f0c) * indra/llcommon/llversionviewer.h (fc82190a3f0c) * indra/llmath/llbbox.h (fc82190a3f0c) * indra/llmath/llbbox.cpp (fc82190a3f0c) * indra/llmessage/llregionflags.h (fc82190a3f0c
Re: [opensource-dev] Review Request: VWR-24254: Add support for using ld.gold on linux.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/48/ --- (Updated 2010-12-21 07:59:33.568556) Review request for Viewer. Changes --- Attached an incremental diff that reverts the removal of APRUTIL_INCLUDE_DIR Summary --- To use ld.gold configure with: -DCMAKE_EXE_LINKER_FLAGS:STRING=-Wl,-use-gold. See VWR-24254 for more details. This addresses bug VWR-24254. http://jira.secondlife.com/browse/VWR-24254 Diffs (updated) - indra/cmake/LLCommon.cmake b0689af42a71 Diff: http://codereview.secondlife.com/r/48/diff Testing --- ld.gold links the viewer on my machine in 8 seconds, as opposed to 19 seconds with ld.bfd. Moreover, it uses a LOT less memory during linking (about 750 MB instead of 2.5 GB! (for viewer 1)). Since my machine locked up hard when I run out of memory (something with using an encrypted RAID for my swap), and compiling viewer 2 uses more than 3 GB, I couldn't compile viewer 2 at all anymore without this patch (and using ld.gold). And OMG this is fast! Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: VWR-24252: Find Qt4 with find_package on STANDALONE.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/46/ --- (Updated 2010-12-20 06:19:04.960312) Review request for Viewer. Changes --- The new version is the same as the previous one, but it adds indra/cmake/FindLLQtWebkit.cmake that was erroneously missing in the previous patch. Summary (updated) --- This patch has only effect on standalone. It searches for the Qt libs using the cmake provided FindQt4.cmake and search for llqtwekbit using the new FindLLQtWebkit.cmake and then uses the resulting LLQTWEBKIT_LIBRARY and LLQTWEBKIT_INCLUDE_DIR in the right places. I added an explicit test to check if QTDIR is set, it is set to the correct value (even LL got this wrong on their wiki, so it's apparently not obvious): As of Qt 4, Qt is found by calling qmake, NOT by looking at QTDIR. So, if you set QTDIR then it better be set to whatever qmake was found (using PATH as usual). Finally, we also need to explicitly pass the Qt plugin libraries in order to link with them, so part of this patch adds the rules to do that (the foreach). Note that if you installed your libs in a non-standard place, then of course you still have to set LD_LIBRARY_PATH yourself before running the viewer ;) This addresses bug VWR-24252. http://jira.secondlife.com/browse/VWR-24252 Diffs (updated) - indra/cmake/FindLLQtWebkit.cmake PRE-CREATION indra/cmake/WebKitLibPlugin.cmake b0689af42a71 indra/llplugin/CMakeLists.txt b0689af42a71 indra/media_plugins/webkit/CMakeLists.txt b0689af42a71 Diff: http://codereview.secondlife.com/r/46/diff Testing --- I used this while working on adding plugin support to Imprudence, needing to debug into the Qt libs and llqtwebkit. I added support for multiple versions of llqtwebkit to imprudence (not just the one needed, but also newer versions of llqtwebkit). Needless to say that I needed this patch to work to find my (several) installations of Qt and llqtwebkit. Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24261: Configuration with cmake 2.8 is extremely slow
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/49/ --- Review request for Viewer. Summary --- Work around for bug in cmake 2.8+ that causes the time to (re)configure the viewer to become 43 times slower. This patch adds a FindZLIB.cmake that doesn't have the bug added in 2.8 (cmake is aware of it and will fix it in a future release, although there is no reason to stop using our own in the future). This FindZLIB is linux specific (we only need it for standalone), see the discussion in SNOW-751. This addresses bug VWR-24261. http://jira.secondlife.com/browse/VWR-24261 Diffs - indra/cmake/CMakeLists.txt b0689af42a71 indra/cmake/FindZLIB.cmake PRE-CREATION Diff: http://codereview.secondlife.com/r/49/diff Testing --- Already added to Snowglobe 1.4 and 1.5. See http://jira.secondlife.com/browse/SNOW-751 Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: VWR-24247: develop.py configure still searches for the wrong header file when checking for Tut
On 2010-12-19 04:03:39, Boroondas Gupte wrote: indra/cmake/FindTut.cmake, line 10 http://codereview.secondlife.com/r/39/diff/1/?file=101#file101line10 According to the CMake 2.8.1 man page, NO_SYSTEM_ENVIRONMENT_PATH makes find_path not only skip $PATH but also $INCLUDE. I think we should respect the latter when looking for headers. I was aware of that, but I don't think that INCLUDE is ever used on linux. Not only not for any existing project, but also gcc doesn't honor it. It can hardly be considered to be a 'standard' (useful) search path for *system* header files when the default compiler isn't even using that environment variable. My guess was that this is more of a Windows(tm) thing. And indeed, Michelle just told me that on windows the command line compiler needs %INCLUDE% to function. So, I think that cmake searches PATH and INCLUDE because of Windows(tm). For example, Windows(tm) sets INCLUDE=C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE;C:\Program Files\Microsoft SDKs\Windows\v6.0A\Include. However, this code is only run on linux, where INCLUDE is not used, and it might even be harmful to search in PATH for header files. Like I said before, it probably won't hurt in this case (it's unlikely that anyone has an executable installed in their PATH called 'tut/tut.hpp') but I don't consider it to be the right thing for linux. That being said - in this particular case (for tut/tut.hpp) it will work just as fine with or without, so if that is a showstopper for Oz (or whoever has to add this patch) than I'd be ok with removing NO_SYSTEM_ENVIRONMENT_PATH and gambling that nothing will be found in PATH, instead of using the patch as-is: ignoring the windows INCLUDE on linux just like gcc ignores it. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/39/#review49 --- On 2010-12-18 09:01:35, Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/39/ --- (Updated 2010-12-18 09:01:35) Review request for Viewer. Summary --- There is no #include tut.h anywhere. The only includes are #include tut/tut.hpp (which is correct). Tut installs in prefix/include/tut/* among which the headerfile tut.hpp. The changed file, indra/cmake/FindTut.cmake, is only included when configured with --standalone (and thus only on linux). find_path searches in /usr/include and /usr/local/include anyway, so there is no way to add that. Adding the NO_SYSTEM_ENVIRONMENT_PATH stops it from reading the PATH environment variable and looking for tut/tut.hpp, which probably won't harm, but it simply makes no sense: we're looking for a headerfile, not an executable. Without this patch (and without creating a fake /usr/local/include/tut.h), I get the error: CMake Error at cmake/FindTut.cmake:26 (message): Could not find Tut Call Stack (most recent call first): cmake/Tut.cmake:8 (include) llmessage/CMakeLists.txt:13 (include) With the patch, it finds /usr/local/include/tut/tut.hpp as expected, setting TUT_INCLUDE_DIR to /usr/local/include, and it also finds (in my case) /usr/src/secondlife/viewers/snowstorm/viewer-development/include/tut/tut.hpp, setting TUT_INCLUDE_DIR to /usr/src/secondlife/viewers/snowstorm/viewer-development/include, since I have a symbolic link from /usr/src/secondlife/viewers/snowstorm/viewer-development/include/tut to ../linden/libraries/include/tut (which was installed manually with ./scripts/install.py tut) and I have the environment variable CMAKE_INCLUDE_PATH set to /usr/src/secondlife/llqtwebkit/install-imprudence/include:/usr/src/secondlife/viewers/snowstorm/viewer-development/include:/sl/usr/include. In other words, this allows developers to install headers whereever they want and use the API of cmake as it is intended, to find those headers. This addresses bug VWR-24247. http://jira.secondlife.com/browse/VWR-24247 Diffs - indra/cmake/FindTut.cmake b0689af42a71 Diff: http://codereview.secondlife.com/r/39/diff Testing --- Tested to see if it finds the header when installed in /usr/local/include as well in a path specified with CMAKE_INCLUDE_PATH. Note that you need to configure with --standalone in order to test this. Also note that unless tut is installed in /usr/local, and when you configure with -DLL_TESTS:BOOL=ON, it still won't compile because of another bug. I will submit a patch for that separately. Thanks, Aleric ___ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource
[opensource-dev] Review Request: VWR-24251: Fix -DLL_TESTS:BOOL=ON on standalone when Tut is installed in a non-standard directory.
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/44/
---
Review request for Viewer.
Summary
---
If tut/tut.hpp isn't installed in a standard include directory all tests
fail because the found include directory for tut isn't passed to the compiler.
This patch fixes this by passing it.
Note that using include_directories() in a Find*.cmake file is bad practise.
The correct way is to set an include dir variable and call
include_directories() once. It certainly doesn't work for the tests anyway
because the tests are all over the place and include_directories is on a
per folder basis. What is needed is to set it for each (test) target.
However, there is no TARGET_INCLUDE_DIRECTORIES. The closest thing that we
have is to set the COMPILE_FLAGS property for a target.
Fortunately, standalone is only used for linux, so we can just use
-I${TUT_INCLUDE_DIR} to get the effect we want.
This addresses bug VWR-24251.
http://jira.secondlife.com/browse/VWR-24251
Diffs
-
indra/cmake/LLAddBuildTest.cmake b0689af42a71
indra/cmake/Tut.cmake b0689af42a71
indra/test/CMakeLists.txt b0689af42a71
Diff: http://codereview.secondlife.com/r/44/diff
Testing
---
Tested with standalone and tut.hpp installed in a non-standard place *after
applying VWR-24247* of course.
All tests compile and pass (on linux 64bit).
Thanks,
Aleric
___
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
[opensource-dev] Review Request: VWR-10579: Fix NDOF.cmake to do the right thing on standalone.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/45/ --- Review request for Viewer. Summary --- Only define -DLIB_NDOF=1 when NDOF is actually found, on standalone. While FindNDOF.cmake was added to the repository, it wasn't used. This addresses bug VWR-10579. http://jira.secondlife.com/browse/VWR-10579 Diffs - indra/cmake/NDOF.cmake b0689af42a71 Diff: http://codereview.secondlife.com/r/45/diff Testing --- I can't remember when I started using this, but it was long ago. I don't have libndofdev installed. I just installed Michelle's debian package libndofdev-dev and then it found it: -- Found NDOF: Library in '/usr/lib/libndofdev.so' and header in '/usr/include' Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24252: Find Qt4 with find_package on STANDALONE.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/46/ --- Review request for Viewer. Summary --- This patch has only effect on standalone. It searches for the Qt libs using the cmake provided FindQt4.cmake. I added an explicit test to check if QTDIR is set, it is set to the correct value (even LL got this wrong on their wiki, so it's apparently not obvious): As of Qt 4, Qt is found by calling qmake, NOT by looking at QTDIR. So, if you set QTDIR then it better be set to whatever qmake was found (using PATH as usual). While FindLLQtWebkit.cmake was added to the repository, it wasn't used for some reason... but it works perfectly. This patch uses it to find LLQtWebkit and then uses the resulting LLQTWEBKIT_LIBRARY and LLQTWEBKIT_INCLUDE_DIR in the right places. Finally, we also need to explicitly pass the Qt plugin libraries in order to link with them, so part of this patch adds the rules to do that (the foreach). This addresses bug VWR-24252. http://jira.secondlife.com/browse/VWR-24252 Diffs - indra/cmake/WebKitLibPlugin.cmake b0689af42a71 indra/llplugin/CMakeLists.txt b0689af42a71 indra/media_plugins/webkit/CMakeLists.txt b0689af42a71 Diff: http://codereview.secondlife.com/r/46/diff Testing --- I used this while working on adding plugin support to Imprudence, needing to debug into the Qt libs and llqtwebkit. I added support for multiple versions of llqtwebkit to imprudence (not just the one needed, but also newer versions of llqtwebkit). Needless to say that I needed this patch to work to find my (several) installations of Qt and llqtwebkit. Thanks, Aleric ___ 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
[opensource-dev] Review Request: SNOW-240: Fix libjson naming madness, for standalone.
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/47/
---
Review request for Viewer.
Summary
---
On linux (and remember this is about standalone)
the libjson packages of distributions don't have this
complex compiler version baked into their name.
This patch fixes this issue by first searching for
libjson_linux-gcc-${_gcc_COMPILER_VERSION}_libmt.so
and when that fails search for the system package
library file libjson.so.
This addresses bug SNOW-240.
http://jira.secondlife.com/browse/SNOW-240
Diffs
-
indra/cmake/FindJsonCpp.cmake b0689af42a71
Diff: http://codereview.secondlife.com/r/47/diff
Testing
---
It works :p (I have Michelle's debian package libjsoncpp0 installed, which
provides /usr/lib/libjson.so).
Thanks,
Aleric
___
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
[opensource-dev] Review Request: VWR-24254: Add support for using ld.gold on linux.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/48/ --- Review request for Viewer. Summary --- To use ld.gold configure with: -DCMAKE_EXE_LINKER_FLAGS:STRING=-Wl,-use-gold. See VWR-24254 for more details. This addresses bug VWR-24254. http://jira.secondlife.com/browse/VWR-24254 Diffs - indra/cmake/BerkeleyDB.cmake b0689af42a71 indra/cmake/LLCommon.cmake b0689af42a71 indra/cmake/LLPlugin.cmake b0689af42a71 indra/llwindow/CMakeLists.txt b0689af42a71 Diff: http://codereview.secondlife.com/r/48/diff Testing --- ld.gold links the viewer on my machine in 8 seconds, as opposed to 19 seconds with ld.bfd. Moreover, it uses a LOT less memory during linking (about 750 MB instead of 2.5 GB! (for viewer 1)). Since my machine locked up hard when I run out of memory (something with using an encrypted RAID for my swap), and compiling viewer 2 uses more than 3 GB, I couldn't compile viewer 2 at all anymore without this patch (and using ld.gold). And OMG this is fast! Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: VWR-24254: Add support for using ld.gold on linux.
On 2010-12-19 11:07:57, Boroondas Gupte wrote: indra/cmake/LLCommon.cmake, line 11 http://codereview.secondlife.com/r/48/diff/1/?file=113#file113line11 Is this removal related to ld.gold, too, or just general cleanup? Oops. That was supposed to be a general cleanup, inspired by the fact that llcommon isn't including apu.h (the header that is being looked for for APRUTIL_INCLUDE_DIR). But because of your question I double checked, and although /usr/include/apr-1.0 contains apr_*.h files and apu_*.h files, about half of the apr_*.h files belong to aprutil! And we DO use those (ie, apr_base64.h). I'll change the patch to not remove that tomorrow. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/48/#review52 --- On 2010-12-19 09:42:53, Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/48/ --- (Updated 2010-12-19 09:42:53) Review request for Viewer. Summary --- To use ld.gold configure with: -DCMAKE_EXE_LINKER_FLAGS:STRING=-Wl,-use-gold. See VWR-24254 for more details. This addresses bug VWR-24254. http://jira.secondlife.com/browse/VWR-24254 Diffs - indra/cmake/BerkeleyDB.cmake b0689af42a71 indra/cmake/LLCommon.cmake b0689af42a71 indra/cmake/LLPlugin.cmake b0689af42a71 indra/llwindow/CMakeLists.txt b0689af42a71 Diff: http://codereview.secondlife.com/r/48/diff Testing --- ld.gold links the viewer on my machine in 8 seconds, as opposed to 19 seconds with ld.bfd. Moreover, it uses a LOT less memory during linking (about 750 MB instead of 2.5 GB! (for viewer 1)). Since my machine locked up hard when I run out of memory (something with using an encrypted RAID for my swap), and compiling viewer 2 uses more than 3 GB, I couldn't compile viewer 2 at all anymore without this patch (and using ld.gold). And OMG this is fast! Thanks, Aleric ___ 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
[opensource-dev] Review Request: VWR-24247: develop.py configure still searches for the wrong header file when checking for Tut
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/39/ --- Review request for Viewer. Summary --- There is no #include tut.h anywhere. The only includes are #include tut/tut.hpp (which is correct). Tut installs in prefix/include/tut/* among which the headerfile tut.hpp. The changed file, indra/cmake/FindTut.cmake, is only included when configured with --standalone (and thus only on linux). find_path searches in /usr/include and /usr/local/include anyway, so there is no way to add that. Adding the NO_SYSTEM_ENVIRONMENT_PATH stops it from reading the PATH environment variable and looking for tut/tut.hpp, which probably won't harm, but it simply makes no sense: we're looking for a headerfile, not an executable. Without this patch (and without creating a fake /usr/local/include/tut.h), I get the error: CMake Error at cmake/FindTut.cmake:26 (message): Could not find Tut Call Stack (most recent call first): cmake/Tut.cmake:8 (include) llmessage/CMakeLists.txt:13 (include) With the patch, it finds /usr/local/include/tut/tut.hpp as expected, setting TUT_INCLUDE_DIR to /usr/local/include, and it also finds (in my case) /usr/src/secondlife/viewers/snowstorm/viewer-development/include/tut/tut.hpp, setting TUT_INCLUDE_DIR to /usr/src/secondlife/viewers/snowstorm/viewer-development/include, since I have a symbolic link from /usr/src/secondlife/viewers/snowstorm/viewer-development/include/tut to ../linden/libraries/include/tut (which was installed manually with ./scripts/install.py tut) and I have the environment variable CMAKE_INCLUDE_PATH set to /usr/src/secondlife/llqtwebkit/install-imprudence/include:/usr/src/secondlife/viewers/snowstorm/viewer-development/include:/sl/usr/include. In other words, this allows developers to install headers whereever they want and use the API of cmake as it is intended, to find those headers. This addresses bug VWR-24247. http://jira.secondlife.com/browse/VWR-24247 Diffs - indra/cmake/FindTut.cmake b0689af42a71 Diff: http://codereview.secondlife.com/r/39/diff Testing --- Tested to see if it finds the header when installed in /usr/local/include as well in a path specified with CMAKE_INCLUDE_PATH. Note that you need to configure with --standalone in order to test this. Also note that unless tut is installed in /usr/local, and when you configure with -DLL_TESTS:BOOL=ON, it still won't compile because of another bug. I will submit a patch for that separately. Thanks, Aleric ___ 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
[opensource-dev] Review Request: scripts/install.py --uninstall does not always remove symbolic links.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/29/ --- Review request for Viewer. Summary --- Packages (tar balls) installed with scripts/install.py do contain symbolic links. Everything that they contain is written to the file installed.xml, and upon uninstall attempted to remove. However, the python script first tests if a file exists before it removes it and uses os.path.exists for this, which only returns true when the target is a file, or a symbolic link *pointing* to an existing file. Since the removal of the tar ball elements is arbitrary, it is possible (and often the case) that the file the symbolic link points to is removed before the symbolic link itself is removed, causing the test to fail and the symbolic link not to be removed. This patch solves this bug by using os.path.lexists which returns true for normal files when they exist, and true for symbolic links if they exist, whether or not the file they point to exists, exactly what we want. os.path.lexists was added to python 2.4, so that should not be problem. This addresses bug SNOW-744. http://jira.secondlife.com/browse/SNOW-744 Diffs - scripts/install.py b0689af42a71 Diff: http://codereview.secondlife.com/r/29/diff Testing --- This patch was originally tested to work several months ago, and has been in use by the Imprudence TPV for a while now. Thanks, Aleric ___ 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
Re: [opensource-dev] Review Request: Settings.xml: redundant entries and unnecessary tag
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/18/#review32 --- ..testing the review system (hi mom!).. indra/newview/app_settings/settings.xml http://codereview.secondlife.com/r/18/#comment15 Considering the setting name (CacheLocationTopFolder), isn't the deleted Comment string better than the one you left in? Thus, Controls the top folder location of the the local disk cache, rather than Controls the location of the local disk cache. Diff is ok with me, I just wondered if you saw that it wasn't an exact duplicate. indra/newview/app_settings/settings.xml http://codereview.secondlife.com/r/18/#comment16 This deletes the entry with 60 seconds between printing the FPS and leaves the one that has 10 seconds between printing. Which is the correct one? indra/newview/app_settings/settings.xml http://codereview.secondlife.com/r/18/#comment17 This really looks wrong. The first OutBandwidth has Comment that has nothing to do with OutBandwidth and a Type Boolean. The one that was left in seem to be the real OutBandwidth looking at the Comment, and has Type F32. My guess is that the deleted entry should not have been deleted, but that the key of this entry is wrong, and that the correct way to fix this is to change the key into whatever it was intended to be. - Aleric On 2010-12-14 12:39:34, Jonathan Yap wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/18/ --- (Updated 2010-12-14 12:39:34) Review request for Viewer. Summary --- I wrote two programs that use settings.xml to produce this massive table: http://wiki.secondlife.com/wiki/Debug_Settings While doing this I found 4 places with duplicate entries and 1 entry that is repeated 4 times. There is also a pair of unnecessary tags. Having these cleaned out would make running my program, and thus updating the wiki table, easier. I've erased all but the last entry for these redundant debug settings. This addresses bug vwr-24100. http://jira.secondlife.com/browse/vwr-24100 Diffs - indra/newview/app_settings/settings.xml 46a990f8296f Diff: http://codereview.secondlife.com/r/18/diff Testing --- Thanks, Jonathan ___ 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
Re: [opensource-dev] Review Request: STORM-524 : Refresh L$ balance
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/6/#review37 --- Ship it! 99% chance that this is correct (not counting the added line with trailing white space ;). Something that can impossibly be harmful, but what made me wonder, is that before getChildLLTextBox(balance) never has setVisible called in the old code, but in the new code apparently needs it (definitely looking logical from the context where it is added). But I suppose that it's irrelevant. - Aleric On 2010-12-10 16:21:57, Merov Linden wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/6/ --- (Updated 2010-12-10 16:21:57) Review request for Viewer. Summary --- Since the L$ balance can be updated by events outside the viewer entirely (purchase of L$ on web) and since we do not pool or receive notifications for this, I implemented a simple click to refresh event on the LLTextBox holding the L$ balance value (proposed by Q in JIRA). I modified the tooltip to reflect this new functionality. This addresses bug STORM-524. http://jira.secondlife.com/browse/STORM-524 Diffs - indra/newview/llbuycurrencyhtml.cpp 8668b574c1f3 indra/newview/llfloaterbuycurrency.cpp 8668b574c1f3 indra/newview/llfloaterbuycurrencyhtml.cpp 8668b574c1f3 indra/newview/llstatusbar.h 8668b574c1f3 indra/newview/llstatusbar.cpp 8668b574c1f3 indra/newview/skins/default/xui/en/panel_status_bar.xml 8668b574c1f3 Diff: http://codereview.secondlife.com/r/6/diff Testing --- Thanks, Merov ___ 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
Re: [opensource-dev] Review Request: Crash in LLRemoteParcelInfoProcessor::processParcelInfoReply()
On 2010-12-16 11:11:33, Kitty Barnett wrote: *confuzzled* Does it want an incremental change starting at the previous diff? Or a diff that includes the original diff (starting from viewer-dev)? When I click on the link 'Diff r2', I don't get a diff - I get an error message that 2 our of 5 hunks failed to apply. - Aleric --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/24/#review42 --- On 2010-12-16 11:06:44, Kitty Barnett wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/24/ --- (Updated 2010-12-16 11:06:44) Review request for Viewer. Summary --- erase() on a multimap will only invalidate iterators that point to the element being erased so pre-incrementing the loop iterator should prevent it from getting invalidated when an observer calls removeObserver() as part of its processParcelInfo() implementation. This addresses bug VWR-24207. http://jira.secondlife.com/browse/VWR-24207 Diffs - indra/newview/llremoteparcelrequest.cpp UNKNOWN Diff: http://codereview.secondlife.com/r/24/diff Testing --- Thanks, Kitty ___ 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
Re: [opensource-dev] [HELP] How can I find/copy GPU settings?
In indra/newview/llfeaturemanager.cpp LLFeatureManager::loadGPUClass reads the file GPU_TABLE_FILENAME and LLFeatureManager::parseGPUTable matches the string 'renderer' with it. You could start with printing 'renderer' and see if it's the same on windows and Mac and if not find out why not. If on Mac it makes sense nevertheless, you'd have to find out why gGLManager.getRawGLString() isn't returning the right thing. As a work around you could hardcode the correct string into your viewer. On Fri, Dec 17, 2010 at 1:09 AM, Trilo Byte trilobyte5...@gmail.com wrote: I'm having some difficulty with the Second Life Viewer and my shiny new GPU (nVidia Quadro 4000). The Mac client doesn't recognize it, and gives me an error when I launch the Viewer app. What's worse, dynamic shadows and deferred rendering are completely broken (I get the same complete system crash that ATI users get). However, the Windows client does not appear to have a problem recognizing the card. I get no error, and I have no problem with the deferred renderer or with any of the shadow settings. I've checked against release Viewers, Snowstorm snapshots, and the Mesh Project snapshots all with the same result. Mac clients can't ID or support the card, Windows clients can. Is there someplace I can go looking around within the Windows client (and support files) to try and find what's needed to detect my GPU so I can rig up Mac support? I'd appreciate any help/ideas Cheers TriloByte Zanzibar ___ 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 ___ 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
Re: [opensource-dev] [HELP] How can I find/copy GPU settings?
On Fri, Dec 17, 2010 at 2:48 AM, Aleric Inglewood aleric.inglew...@gmail.com wrote: In indra/newview/llfeaturemanager.cpp LLFeatureManager::loadGPUClass reads the file GPU_TABLE_FILENAME and LLFeatureManager::parseGPUTable matches the string 'renderer' with it. You could start with printing 'renderer' and see if it's the same on windows and Mac and if not find out why not. If on Mac it makes sense nevertheless, you'd have to find out why gGLManager.getRawGLString() isn't returning the right thing. As a work around I meant.. if it makes sense, you could add a line to gpu_table.txt Also, if this fails you should get a warning in the debug output that prints renderer: LL_WARNS(RenderInit) Couldn't match GPU to a class: gGLManager.getRawGLString() LL_ENDL; you could hardcode the correct string into your viewer. On Fri, Dec 17, 2010 at 1:09 AM, Trilo Byte trilobyte5...@gmail.com wrote: I'm having some difficulty with the Second Life Viewer and my shiny new GPU (nVidia Quadro 4000). The Mac client doesn't recognize it, and gives me an error when I launch the Viewer app. What's worse, dynamic shadows and deferred rendering are completely broken (I get the same complete system crash that ATI users get). However, the Windows client does not appear to have a problem recognizing the card. I get no error, and I have no problem with the deferred renderer or with any of the shadow settings. I've checked against release Viewers, Snowstorm snapshots, and the Mesh Project snapshots all with the same result. Mac clients can't ID or support the card, Windows clients can. Is there someplace I can go looking around within the Windows client (and support files) to try and find what's needed to detect my GPU so I can rig up Mac support? I'd appreciate any help/ideas Cheers TriloByte Zanzibar ___ 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 ___ 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
Re: [opensource-dev] Blocking viewers.
I'm not happy to say it, but I can't help myself... I told you so When LL announced the TPV list, it was already clear to me that they want to control what viewer can connect and that this was the beginning of whitelist. Soon every viewer that is not derived from their holy 2.0 will be blocked. On Wed, Sep 8, 2010 at 11:39 PM, Tom Grimshaw t...@streamsense.net wrote: Dear Linden Lab, It's absolutely none of your business what software I choose to run on my PC. Blocking emerald is a step of pure arrogance - and ignorance - on Linden Lab's behalf - it's not having an adverse effect on your servers, in fact THE ONLY WAY you can tell i'm runing Emerald is by the channel and version provided in login, and this has been proven by the number of clones that have popped up with their channel renamed (and the ID texture changed of course). You cannot censor Open Source software, and the fact that you're trying to makes you a despicable organisation. Stop policing my computer. I will decide what viewer I use, thank you. ~T ___ 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 ___ 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
[opensource-dev] This is my goodbye
I've compared Linden Lab more often with an abusive husband. You take the beating and then believe the stories and hope that it all will become better. It's hard to leave such a husband usually, because you invested in that life and one lose a lot when one leaves. Nevertheless, I have come to the conclusion that I've been a fool in the past. I've been mad, over and over and over again. I've told myself every time: this is unacceptable; but still I believed things would change for the better and came back... not really full force (in the end the number of patches that I actually wrote are a fraction of what I'd have done when I hadn't been beaten up on a regular basis). Today, still with a black eye of my homestead being taken offline, I have been kicked in the face by the hard cut off of snowglobe. Yes, it was inevitable that in the end all viewers would be derived from the official viewer (aka, 2.x), but NOT by force! It should have been as a result of new features. Open source should be the evolution of ideas (and code) and not being enforced and dictated. Slowly I have began to realize that Linden Lab is a control freak. They try to control EVERYTHING, from what you do in-world to the what name your viewer can have, or what you run on your PC to connect to their servers. They have never listened to the users before making important decisions (and seldom afterwards) and I have seen no recent change in that. I do think that the snowstorm change was a good change, but in the end it is just abuse of open source volunteers by giving them the exact same treatment as paid Linden Lab employees: all the hard work, *exactly* following the rules dictated by Linden Lab but no room to do it your way. No voice in any decision making, and certainly no room to experience, or freedom. Open source, however, is about FREEDOM: The freedom to CHANGE the code and to USE it as you see fit. The Best Ideas then survive as a result of evolution. Bad ideas aren't used, popular ideas are copied by everyone. Linden Lab makes this natural process impossible. The only thing that is left is working for a company, without freedom, without a vote and without being paid. I'm sorry that this how they run their business. I'm a lot more sorry that I saw too late that I really shouldn't volunteer to work for them under those conditions. THIIS NOT OPEN SOURCE For now I have joined imprudence (http://imprudenceviewer.org/). This is a true open source project. Surely Linden Lab can still block it from their servers, and they WILL do so when they start to disallow 1.23, but at least I won't be working anymore for them. Patches contributed to Imprudence will be pure GPL and not fall under the Contribution Agreement. Because this project is really, purely GPL it will be possible to use OTHER GPL-ed code, from other third party viewers for example, or anywhere else from the net. THAT is what open source is about. I'm pleased to have noticed that all the important open source people that I have learned to appreciate and respect are already there. If you are not, please join us! This doesn't have to a goodbye: I hope to see you join us on the other side! Thank you for your time, Aleric Inglewood. PS CC to imprudence mailinglist ___ 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
Re: [opensource-dev] Testing for VWR-20741 and sub task VWR-20933
Hi, I didn't actually test it, but looked at the changeset in a review-kind-of-way, and have the following concerns: 1) It has been agreed in the past, imho, that this would be turned off by default; the current patch turns it on. 2) If there has communication with people with this turned off, and then the option is turned on; then the last part of the communication with the option turned off is shown till eternity if there wasn't some chat history today or yesterday; which is basically the same concern as point 3: 3) If there was communication -say- three days ago (but not today or yesterday), then that isn't found. A solution to point 2 and 3 would be to create a symbolic link to the last log file created and look for that instead of oldLogFileName The name of this symbolic link may not contain a date of course, and it may not be equal to ndsLogFileName. Ie, it could have -most_recent appended. On Wed, Sep 1, 2010 at 1:44 PM, WolfPup Lowenhar wolfpu...@earthlink.net wrote: I requesting of my fellow open source community members assistance in testing the following: http://jira.secondlife.com/browse/VWR-20741 and sub-task: http://jira.secondlife.com/browse/VWR-20933 The change set for testing is here: http://bitbucket.org/WolfpupL/viewer-development/changeset/36caec874c28 ___ 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 ___ 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
Re: [opensource-dev] Removal of the MultipleAttachments debug settings ?
The following has been proposed before: * Add new bits to each object (all existing objects should act as if all bits are set). * Give the bits a default meaning (read: human readable word, which can be different per attachment point), but allow each user to override those descriptions locally. * Allow users to change the bits for each of their objects, even no-modify ones. * If a user 'wears' (or adds, which becomes redundant) a new attachment, then remove those attachments that have one or more of the same bits set. In other words, at any time one can only have one object attached at a given position with any given bit set. Suppose you think that 8 bits are enough, then the following holds: * = old 'wear' behavior: replaces everything else. * = 'add' behavior: is added, replaces nothing. * 0001 = (for example): assign default meaning 'jacket' for chest attachments (jacket collars and hoodies). * 0010 = (for example): assign default meaning 'shirt' for chest attachments (shirt collars). * 0100 = (for example): assign default meaning 'necklace' for chest attachments. and so on. This allows users to make groups of attachments that are mutually exclusive, but having up till 8 classes that can be worn at the same time on the same attachment point. Personally I think that those bits also should be added to normal wearables, so that it is possible to have attachments being removed when you wear a new shirt (ie, a shirt without a collar should remove all existing shirt-collars, or wearing a penis could automatically remove underwear and pants and visa versa, Linden shoes could remove prim shoes, etc, all user customizable for his/her own attachments; the default naming would be just a hint to make things work reasonable after just having bought it). I'm not sure, but I think that having eight classes per attachment points should be enough, so adding a single byte to every object should be enough. On Thu, Aug 26, 2010 at 10:43 PM, Nyx Linden n...@lindenlab.com wrote: however, so if you have suggestions for better ways of exposing the functionality, please do let us know! ___ 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
Re: [opensource-dev] Removal of the MultipleAttachments debug settings ?
I fail to see how your example could undermine the bits-proposal. For a start, the bits *only* add possibilities (254 per object!). Currently the ONLY existing behavior is as if my bits are already in place, but set to when you 'wear' something, and to when you 'add' something. Adding the bits will just open up a whole scala of control over how your attachments replace other attachments (and clothing?). On Fri, Aug 27, 2010 at 2:20 PM, Francesco Rabbi syt...@gmail.com wrote: You lost quite all good taste of this feature Ppl should be free to add everything in funny/weird way, and creators cannot be limited matching which bit another creator use... The possibilities only increase, as I just explained. You can still add anything in every way possible. I specifically said that each user should be able to change the bits of objects even if those objects are no modify. You can always just set all bits to 0, then you can wear anything and everything at the same time. The solution is resident side: outfits folders, may be interesting mark boxes or vendors to create automatically outfit folders maybe Lets look at your example. I added comment on the right. Example: -outfit naked Hair --- Head attachament SHAPE NO bits (or all 0) Skin NO bits (or all 0) Genitalia --- Pelvis attachment -outfit sport All above but genitalia Jeans--- covers genitalia Shirt Shirt collar --- Chest attachment Necklace --- Chest attachment Shoes --- Feet attachments Ok, so a user could set the same bit on the Genitalia object as on the pants. Then they would become mutual exclusive, which is what you want apparently (and which makes sense since pants cover genitalia. Pants that don't should just NOT set that bit). Assuming that it looks good if you wear the necklace and the shirt collar at the same time: put them in different classes so they won't replace each other when worn. replace outfit should revert from to other, without got creators crazy replace outfit is an entirely different thing than wear attachment/clothing. The usual meaning is that EVERYTHING is removed and then things in the folder are added. If a vendor or box can be enganced with outfit (over buy content and buy copy) I think daily outfit management by the users is more important than the urge to control how and what the users wear by vendors. If things bought by users don't work out of the box, then they will have to manually remove a few things, just like now. But at least they'll be able to automate that and not have to worry about having the add and remove attachments/clothing EVERY time, when wearing stuff from inventory. Any change to wearing a new outfit that is in a just-bought box is orthogonal to my proposal. ( all this marked with a giant IMHO) -- Sent by iPhone Il giorno 27/ago/2010, alle ore 14:11, Aleric Inglewood aleric.inglew...@gmail.com ha scritto: The following has been proposed before: * Add new bits to each object (all existing objects should act as if all bits are set). * Give the bits a default meaning (read: human readable word, which can be different per attachment point), but allow each user to override those descriptions locally. * Allow users to change the bits for each of their objects, even no-modify ones. * If a user 'wears' (or adds, which becomes redundant) a new attachment, then remove those attachments that have one or more of the same bits set. In other words, at any time one can only have one object attached at a given position with any given bit set. Suppose you think that 8 bits are enough, then the following holds: * = old 'wear' behavior: replaces everything else. * = 'add' behavior: is added, replaces nothing. * 0001 = (for example): assign default meaning 'jacket' for chest attachments (jacket collars and hoodies). * 0010 = (for example): assign default meaning 'shirt' for chest attachments (shirt collars). * 0100 = (for example): assign default meaning 'necklace' for chest attachments. and so on. This allows users to make groups of attachments that are mutually exclusive, but having up till 8 classes that can be worn at the same time on the same attachment point. Personally I think that those bits also should be added to normal wearables, so that it is possible to have attachments being removed when you wear a new shirt (ie, a shirt without a collar should remove all existing shirt-collars, or wearing a penis could automatically remove underwear and pants and visa versa, Linden shoes could remove prim shoes, etc, all user customizable for his/her own attachments; the default naming would be just a hint to make things work reasonable after just having bought it). I'm not sure, but I think that having eight classes per attachment points should be enough, so adding
[opensource-dev] Snowstorm Backlog Request: SNOW-766
Status: reviewed by Merov, committed to snowglobe 1.4, 1.5 and 2.1. Background: When developing many viewers in parallel (and snowstorm with it's many clones that need to be checked out won't change that), it becomes necessary to automate certain things with scripts. One of the things those scripts need to know is the current build directory. However, the build directory is a function of the configuration of the viewer. In order to remove human maintenance (and possible errors therein) it is desirable to have an automated way to convert configuration to build directory name. I wrote such scripts and they break down with the current viewer-development: hikaru:/usr/src/secondlife/viewers/snowstorm/test-20100818source env.source Error: unknown subcommand 'printbuilddirs' (run 'develop.py --help' for help) CONFIGURE_OPTS = --type=Release -m64 --standalone CMAKE_DEFS = -DLL_TESTS:BOOL=ON -DPACKAGE:BOOL=ON -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON CMAKE_PREFIX_PATH = /sl:/sl/usr CMAKE_INCLUDE_PATH = /usr/src/secondlife/llqtwebkit/install2/include:/usr/src/secondlife/viewers/snowstorm/test-20100818/include:/sl/usr/include CMAKE_LIBRARY_PATH = /usr/src/secondlife/llqtwebkit/install2/lib: Sprint plan: Port this patch to viewer-development for the next sprint and test it. Before patch: hikaru:/usr/src/secondlife/viewers/snowstorm/test-20100818/linden/indra./develop.py --type=Release -m64 --standalone printbuilddirs setting DISTCC_DIR to /usr/src/secondlife/viewers/snowstorm/test-20100818/linden/indra/.distcc Error: unknown subcommand 'printbuilddirs' (run 'develop.py --help' for help) After patch: This should print (on this box): viewer-linux-x86_64-release Please let me know if anything is wrong or missing in this post, Aleric ___ 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
[opensource-dev] Where are the auto builds?
The auto builds (at least for 1.5) seem to have stopped. Can this please be fixed asap? Aleric ___ 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
[opensource-dev] 1.4 Showstopper: SNOW-799
Please your attention for http://jira.secondlife.com/browse/SNOW-799 The only reason I can currently think of that this might not be a showstopper is when it is caused by having 'Use HTTP textures' turned on... but shouldn't that be working with 1.4? ___ 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
[opensource-dev] 1.4 Showstopper: Reopened: VWR-9475
Cursors are missing from the artwork... This was assigned to Soft Linden who closed it as 'resolved', but it is NOT resolved. Should I be confused? Perhaps it should be assigned to Merov? ___ 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
Re: [opensource-dev] [sldev-commits] r3568 - in projects/2010/snowglobe/trunk: . doc indra/llcharacter indra/llcommon indra/llinventory indra/llmessage indra/llrender indra/llui indra/newview indra/ne
I can't think of a reason that adding any files to the repository can or should be blocking someone from doing an update. And since I haven't seen any error messages or explanation, I'm clueless. Seems like a local problem to me. On Tue, Jul 27, 2010 at 8:20 AM, Philippe (Merov) Bossut me...@lindenlab.com wrote: Hi Wolfpup, On Mon, Jul 26, 2010 at 7:54 PM, WolfPup Lowenhar wolfpu...@earthlink.net wrote: I'm no longer able to do the updates thx to the .svnignore files in the svn server as they are blocking TortiousSVN from doing any updates! Sorry about this. You're apparently not the only one with the problem. Using the command line seems to work though so stick with svn update from Cygwin for the moment. I'd like to hear from Aleric on this since he did all of the .svnignore changes recently. Cheers, - Merov ___ 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 ___ 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
[opensource-dev] jira patch name convention
Hi all, the jira makes no difference between Snowglobe 1.4 and Snowglobe 2.1, the two most active development branches at the moment. While many jira entries are specifically about one or the other, some things apply to both. There are two ways to deal with this: either create a new jira for the same thing, so we have two jira entries about the same thing, one for patches related to 1.x and one for patches related to 2.x. However, that is not always feasible. The second option is to use one jira for both and just attach patches (if they differ) for both branches to the same jira. For that latter case I propose to use the following convention: Patches that are for 1.x should start with SNOW1-123_description.diff Patches that are for 2.x should start with SNOW2-123_description.diff Leaving out the version would mean it applies in general. Ie, SNOW-123_description.diff should apply to 1.x and/or 2.x (depending on what the jira is about). If there is a patch that is specifically for SG 2.0, and the patch for 2.1 has to look different, you can call it SNOW20-123_description.diff, etc. Aleric ___ 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
Re: [opensource-dev] Viewer-external to Snowglobe 2.x trunk merge update
I did a few commits: http://svn.secondlife.com/trac/linden/changeset/3486 http://svn.secondlife.com/trac/linden/changeset/3487 http://svn.secondlife.com/trac/linden/changeset/3488 http://svn.secondlife.com/trac/linden/changeset/3489 After that it compiled and linked for me on linux, and probably also on mac... Hopefully you can look into the link problem on mac of the 1.4 trunk too: it needs a new apr tar ball with shared libraries (See my last comments on SNOW-713). I assigned it to you, of course. On Wed, Jul 14, 2010 at 7:27 AM, Philippe (Merov) Bossut me...@lindenlab.com wrote: Hi, Following the Hippo meeting this afternoon and one last test on Windows, I did commit that merge to Snowglobe 2.x trunk: svn r 3484. This is a merge of all viewer-external revisions from svn r3348 to r3472. The Windows build went fine on TeamCity and we do have a Snowglobe 2.1 binary that can be tested: http://secondlife.com/developers/opensource/downloads/2010///Snowglobe_2-1-0-0_Setup.exe If you want to give it a spin, please do! I'm particularly interested in Water Flickering tests as this is the merge code with the most conflicts. Mac and Linux on the other hand fail. I'm working on Mac right now and will get that fixed asap with the highest priority (it looks like I screwed up some viewer_manifest.py stuff in the merge). If you are building on Mac or Linux, I do appreciate your help to fix the build *but*, if you have no time twiddling with build issues, please do not update your repo! Cheers, - Merov On Fri, Jul 9, 2010 at 7:42 PM, Philippe (Merov) Bossut me...@lindenlab.com wrote: Hi, I've been hit by Parabuild to TeamCity move issues today, with the Snowglobe build failing on TeamCity for script reasons. I just got a full 2.x trunk building on all platforms now on TeamCity but I think it's too late to do a big commit merge on a Friday night and have everything broken during the weekend... so I'm delaying that big commit merge to Monday morning. That way, I'll be there to keep an eye on builds and get help on TeamCity if needs be. Hope it's not derailing your plans. Cheers, - Merov On Thu, Jul 8, 2010 at 12:01 PM, Philippe (Merov) Bossut me...@lindenlab.com wrote: Hi, As discussed at the last Hippo meeting, I've been working on this big merge those last few days. After a couple of false starts, I decided to use brute force and do a big range merge from svn r3348 to r3472. Surprise: it worked! Well, modulo some changes (mostly in the Water Flickering fix but also in TP double click and sit) but I do have a Snowglobe 2.1 version working on Windows right now :) I'll be doing some more verifications today (that I didn't really gut some Snowglobe features completely and don't have obvious crashers) and we'll talk about it at the Hippo meeting later this afternoon. If everything goes well though, I'm planning a *big* commit tomorrow morning. Let me know if you have issues with this plan. Cheers, - Merov ___ 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 ___ 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
Re: [opensource-dev] Problems with automatic builds and commit messages to sldev-commits
Ok, right after this post another Failed Build for () on Linux appeared, this time one where apparently that patch was applied. The Processing res/secondlife_icon.png = secondlife_icon.png ... correctly changed into Processing snowglobe_icon.png = snowglobe_icon.png ... However, strangly enough it then barfs on RuntimeError: Path /var/opt/parabuild/checkout/L-snowglobe-20-trunk/linden/indra/viewer-linux-i686-release/newview/snowglobe_icon.png doesn't exist I guess I removed the res/ part erroneously, thinking it had to be removed because I DO have indra/$BUILDDIR/newview/snowglobe_icon.png ... but just now realized that that is a symbolic link that I added in order to run the viewer without packaging it :/. I'll make a new commit that reverts the last hunk of changeset 3489. On Wed, Jul 14, 2010 at 4:42 PM, Aleric Inglewood aleric.inglew...@gmail.com wrote: Hi Merov, can you address these problems please: 1) The automatic builds do not build the latest source. For example, https://lists.secondlife.com/pipermail/sldev-commits/2010-July/004101.html posted on *Wed Jul 14 06:31:59 PDT 2010 *is the last commit, changeset 3489, after which linux builds and works. Yet, after it we get: https://lists.secondlife.com/pipermail/sldev-commits/2010-July/004103.html posted on *Wed Jul 14 06:57:12 PDT 2010*, *25 minutes* later with a link to http://secondlife.com/developers/opensource/downloads/2010///failed-build.Linux containing this error: Processing res/secondlife_icon.png = secondlife_icon.png ... Traceback (most recent call last): File /var/opt/parabuild/checkout/L-snowglobe-20-trunk/linden/indra/newview/viewer_manifest.py, line 1042, in module main() File /var/opt/parabuild/checkout/L-snowglobe-20-trunk/linden/indra/newview/../lib/python/indra/util/llmanifest.py, line 239, in main wm.do(*args['actions']) File /var/opt/parabuild/checkout/L-snowglobe-20-trunk/linden/indra/newview/../lib/python/indra/util/llmanifest.py, line 663, in do self.construct() File /var/opt/parabuild/checkout/L-snowglobe-20-trunk/linden/indra/newview/viewer_manifest.py, line 977, in construct super(Linux_i686Manifest, self).construct() File /var/opt/parabuild/checkout/L-snowglobe-20-trunk/linden/indra/newview/viewer_manifest.py, line 874, in construct self.path(res/+self.icon_name(),self.icon_name()) File /var/opt/parabuild/checkout/L-snowglobe-20-trunk/linden/indra/newview/../lib/python/indra/util/llmanifest.py, line 658, in path count = try_path(os.path.join(self.get_build_prefix(), src)) File /var/opt/parabuild/checkout/L-snowglobe-20-trunk/linden/indra/newview/../lib/python/indra/util/llmanifest.py, line 645, in try_path self.check_file_exists(src) File /var/opt/parabuild/checkout/L-snowglobe-20-trunk/linden/indra/newview/../lib/python/indra/util/llmanifest.py, line 614, in check_file_exists os.path.normpath(os.path.join(os.getcwd(), path)),)) RuntimeError: Path /var/opt/parabuild/checkout/L-snowglobe-20-trunk/linden/indra/viewer-linux-i686-release/newview/res/secondlife_icon.png doesn't exist which I *fixed* in changeset 3489. 2) The posts of the autobuild lacks information * The Subject lines are Failed Build for () on Linux. What should be between the brackets? * After that is always posts Successful Build for () with the happy contents: CYGWIN: http://secondlife.com/developers/opensource/downloads/2010///Snowglobe_2-1-0-0_Setup.exe http://secondlife.com/developers/opensource/downloads/2010///good-build.CYGWIN Darwin: http://secondlife.com/developers/opensource/downloads/2010///Snowglobe_2_0_1_0.dmg http://secondlife.com/developers/opensource/downloads/2010///good-build.Darwin Linux: http://secondlife.com/developers/opensource/downloads/2010///Snowglobe-i686-2.0.1.0.tar.bz2 http://secondlife.com/developers/opensource/downloads/2010///good-build.Linux No change information available EVEN if one or more of those FAILED to build... In all cases, the revision number that should go between the () is missing :/ ___ 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
