From: Stuart B. <stu...@gm...> - 2011-07-27 22:30:13
|
Hi All, I've got a small patch to improve the FG forests, along with some particularly bad C++ I need advice on. The patch does the following: 1) Fixes the longstanding bug where the first set of tree definitions in a tile were used for all forests within that tile. This meant that if you have an area of deciduous forest next to some evergreen, you'd only see one type of tree or the other, depending on which was loaded first. 2) Re-introduce a feature to fade in trees by distance, so that a reduced density is visible at 2*tree-range-m, with full density at tree-range-m. As the default tree-range-m is 8km, this help stop forests "springing up" well after the rest of the tile is perfectly visible. On my machine I don't see any performance impact, despite the fact that more trees are displayed. I'd appreciate it if those with more graphics-constrained systems than my own would test this and let me know if they think the frame-rate hit is acceptable given the improvements in apparent tree coverage. I'd also be interested to hear if tree range is a performance issue in general and people would like control of it in a similar way to the 3D clouds distance slider (if anyone uses that!). The patch is available from http://www.nanjika.co.uk/flightgear/forest.diff If I have time I'll do battle with git and see if I can submit it via gitorious properly... Now, for the bad C++ :) Within the patch is the code below. The (*j)-> just looks ugly. Surely there's a better way? I'm sure those of you who write C++ more regularly than me will immediately be able to tell me where I'm going wrong! -Stuart TreeBin* treebin; SGTreeBinList::iterator j; bool found = false; for (j = randomForest.begin(); (j != randomForest.end()) && (!found); j++) { if (((*j)->texture == mat->get_tree_texture() ) && ((*j)->texture_varieties == mat->get_tree_varieties()) && ((*j)->range == mat->get_tree_range() ) && ((*j)->width == mat->get_tree_width() ) && ((*j)->height == mat->get_tree_height() ) ) { treebin = *j; found = true; } } |
From: Hal V. E. <hv...@gm...> - 2011-07-27 23:00:27
|
On Wednesday, July 27, 2011 03:30:06 PM Stuart Buchanan wrote: > Hi All, > > I've got a small patch to improve the FG forests, along with some > particularly bad C++ I need advice on. > > The patch does the following: > 1) Fixes the longstanding bug where the first set of tree definitions > in a tile were used for all forests within that tile. This meant that > if you have an area of deciduous forest next to some evergreen, you'd > only see one type of tree or the other, depending on which was loaded > first. > 2) Re-introduce a feature to fade in trees by distance, so that a > reduced density is visible at 2*tree-range-m, with full density at > tree-range-m. As the default tree-range-m is 8km, this help stop > forests "springing up" well after the rest of the tile is perfectly > visible. > > On my machine I don't see any performance impact, despite the fact > that more trees are displayed. I'd appreciate it if those with more > graphics-constrained systems than my own would test this and let me > know if they think the frame-rate hit is acceptable given the > improvements in apparent tree coverage. > > I'd also be interested to hear if tree range is a performance issue in > general and people would like control of it in a similar way to the 3D > clouds distance slider (if anyone uses that!). > > The patch is available from http://www.nanjika.co.uk/flightgear/forest.diff > > If I have time I'll do battle with git and see if I can submit it via > gitorious properly... > > Now, for the bad C++ :) > > Within the patch is the code below. The (*j)-> just looks ugly. Surely > there's a better way? > > I'm sure those of you who write C++ more regularly than me will > immediately be able to tell me where I'm going wrong! > > -Stuart > > TreeBin* treebin; > SGTreeBinList::iterator j; > bool found = false; > > for (j = randomForest.begin(); (j != randomForest.end()) && (!found); > j++) { > if (((*j)->texture == mat->get_tree_texture() ) && > ((*j)->texture_varieties == mat->get_tree_varieties()) && > ((*j)->range == mat->get_tree_range() ) && > ((*j)->width == mat->get_tree_width() ) && > ((*j)->height == mat->get_tree_height() ) ) { > treebin = *j; > found = true; > } > } It appears that SGTreeBinList is a list of pointers to some structure. If that is the case then you need to dereference two pointers to get stuff from the structure since the iterator, j, is a pointer. Thus you get *j->texture - *j dereferences j and -> dereferences the pointer pointed to by j. I don't think that you gain anything by writing this as (*j)->texture since this is basically the same thing as *j->texture. IMO *j->texture is easier to read. But there is one minor and very common issue with the code that should be fixed. In the for loop for (..; ..; j++) should be for (..; ..; ++j) if you use j++ the compiler has to make a copy of j with each iteration of the loop but if you use ++j it does not have to make a copy. This will make the loop more efficient although only by a small amount. Hal |
From: Torsten D. <To...@t3...> - 2011-07-28 07:42:14
|
> > TreeBin* treebin; > > > SGTreeBinList::iterator j; > > > bool found = false; > > > > > > for (j = randomForest.begin(); (j != randomForest.end()) && (!found); > > > j++) { > > > if (((*j)->texture == mat->get_tree_texture() ) && > > > ((*j)->texture_varieties == mat->get_tree_varieties()) && > > > ((*j)->range == mat->get_tree_range() ) && > > > ((*j)->width == mat->get_tree_width() ) && > > > ((*j)->height == mat->get_tree_height() ) ) { > > > treebin = *j; > > > found = true; > > > } > > > } > > > It appears that SGTreeBinList is a list of pointers to some structure. > If that is the case then you need to dereference two pointers to get > stuff from the structure since the iterator, j, is a pointer. Thus you > get *j->texture - *j dereferences j and -> dereferences the pointer > pointed to by j. I don't think that you gain anything by writing this as > (*j)->texture since this is basically the same thing as *j->texture. IMO > *j->texture is easier to read. But doesn't work. -> has higher precedence than *. *j->texture is interpreted as *(j->texture) resulting in a compiler error because j has no member named texture. You have to dereference j before accessing the members, hence (*j)->texture. To get rid of that (*j), you can assign a local variable: for (j = randomForest.begin(); (j != randomForest.end()) && (!found); ++j) { TreeBin * treeBin = *j; if( treeBin->texture == mat->get_tree_texture() etc. etc. You have to decide for yourself is this is an improvement ;-) You can also make it worse: (*(*j)j).texture should also work ;-) Torsten Torsten |
From: Stefan S. <ni...@de...> - 2011-07-28 06:25:10
|
On Thursday 28 July 2011 01:00:10 Hal V. Engel wrote: > But there is one minor and very common issue with the code that should be > fixed. In the for loop > > for (..; ..; j++) > > should be > > for (..; ..; ++j) > > if you use j++ the compiler has to make a copy of j with each iteration of > the loop but if you use ++j it does not have to make a copy. This will > make the loop more efficient although only by a small amount. Are you sure about that? I just tried it with a little example and at least gcc compiles both variants to the exact same assembly code. Tried it with and without -O2. Regards, Stefan |
From: James T. <zak...@ma...> - 2011-07-28 06:31:40
|
On 27 Jul 2011, at 23:30, Stuart Buchanan wrote: > Within the patch is the code below. The (*j)-> just looks ugly. Surely > there's a better way? > > I'm sure those of you who write C++ more regularly than me will > immediately be able to tell me where I'm going wrong! As noted elsewhere, you can't avoid the double indirection, but you could use foreach (from Boost): TreeBin* bin = NULL; bool found = false; BOOST_FOREACH(bin, randomForest) { if ((bin->texture() == mat->get_tree_texture()) && ... { found = true; break; } } |
From: Gene B. <ge...@de...> - 2011-07-28 12:34:15
|
On Thu, 28 Jul 2011, Stefan Seifert wrote: > On Thursday 28 July 2011 01:00:10 Hal V. Engel wrote: > >> But there is one minor and very common issue with the code that should be >> fixed. In the for loop >> >> for (..; ..; j++) >> >> should be >> >> for (..; ..; ++j) >> >> if you use j++ the compiler has to make a copy of j with each iteration of >> the loop but if you use ++j it does not have to make a copy. This will >> make the loop more efficient although only by a small amount. > > Are you sure about that? I just tried it with a little example and at least > gcc compiles both variants to the exact same assembly code. Tried it with and > without -O2. > That would freak me out. Doesn't "++j" mean "increment j, then test" whereas "j++" means "test j, then increment"? g. -- Proud owner of F-15C 80-0007 http://www.f15sim.com - The only one of its kind. http://www.simpits.org/geneb - The Me-109F/X Project Some people collect things for a hobby. Geeks collect hobbies. ScarletDME - The red hot Data Management Environment A Multi-Value database for the masses, not the classes. http://www.scarletdme.org - Get it _today_! Political correctness is a doctrine, fostered by a delusional, illogical minority, and rabidly promoted by an unscrupulous mainstream media, which holds forth the proposition that it is entirely possible to pick up a turd by the clean end. |
From: Tim M. <tim...@gm...> - 2011-07-28 14:03:39
|
On Thu, Jul 28, 2011 at 8:33 AM, Gene Buckle <ge...@de...> wrote: > On Thu, 28 Jul 2011, Stefan Seifert wrote: > >> On Thursday 28 July 2011 01:00:10 Hal V. Engel wrote: >> >>> But there is one minor and very common issue with the code that should be >>> fixed. In the for loop >>> >>> for (..; ..; j++) >>> >>> should be >>> >>> for (..; ..; ++j) >>> >>> if you use j++ the compiler has to make a copy of j with each iteration of >>> the loop but if you use ++j it does not have to make a copy. This will >>> make the loop more efficient although only by a small amount. >> >> Are you sure about that? I just tried it with a little example and at least >> gcc compiles both variants to the exact same assembly code. Tried it with and >> without -O2. >> > That would freak me out. Doesn't "++j" mean "increment j, then test" > whereas "j++" means "test j, then increment"? No, in both cases the test is done before the increment. The difference between these cases is that in the post-increment case the initial value of the variable must be saved and returned as the value of the operation. In a typical for-loop that value won't be used, so in simple cases -- such as just incrementing an index or pointer -- the compiler might very well generate the same code. The difference is seen when the variable is an iterator with hairy overloaded functions for the increment operations. Tim > > g. > > -- > Proud owner of F-15C 80-0007 > http://www.f15sim.com - The only one of its kind. > http://www.simpits.org/geneb - The Me-109F/X Project > Some people collect things for a hobby. Geeks collect hobbies. > > ScarletDME - The red hot Data Management Environment > A Multi-Value database for the masses, not the classes. > http://www.scarletdme.org - Get it _today_! > > Political correctness is a doctrine, fostered by a delusional, illogical > minority, and rabidly promoted by an unscrupulous mainstream media, which > holds forth the proposition that it is entirely possible to pick up a turd > by the clean end. > > ------------------------------------------------------------------------------ > Got Input? Slashdot Needs You. > Take our quick survey online. Come on, we don't ask for help often. > Plus, you'll get a chance to win $100 to spend on ThinkGeek. > http://p.sf.net/sfu/slashdot-survey > _______________________________________________ > Flightgear-devel mailing list > Fli...@li... > https://lists.sourceforge.net/lists/listinfo/flightgear-devel > |
From: Jari H. <ja...@fl...> - 2011-07-28 13:19:04
|
On 2011-07-28 14.33, Gene Buckle wrote: > On Thu, 28 Jul 2011, Stefan Seifert wrote: > >> On Thursday 28 July 2011 01:00:10 Hal V. Engel wrote: >> >>> But there is one minor and very common issue with the code that should be >>> fixed. In the for loop >>> >>> for (..; ..; j++) >>> >>> should be >>> >>> for (..; ..; ++j) >>> >>> if you use j++ the compiler has to make a copy of j with each iteration of >>> the loop but if you use ++j it does not have to make a copy. This will >>> make the loop more efficient although only by a small amount. >> >> Are you sure about that? I just tried it with a little example and at least >> gcc compiles both variants to the exact same assembly code. Tried it with and >> without -O2. >> > That would freak me out. Doesn't "++j" mean "increment j, then test" > whereas "j++" means "test j, then increment"? No, for a for loop for ( [1]; [2]; [3] ) where [3] is ++j will increment j before use. However, in an if-statement the complete statement [3] is evaluated before the test [2] is done. If the compiler is smart it will produce the fastest binary code regardless ++j or j++. However, if the [3] is more complicated like a hypothetical i = ++j + k the compiler will most probably generate different binary code (compared to i = ++j + k). Jari |
From: Jari H. <ja...@fl...> - 2011-07-28 13:50:02
|
On 2011-07-28 14.52, Jari Häkkinen wrote: >> That would freak me out. Doesn't "++j" mean "increment j, then test" >> whereas "j++" means "test j, then increment"? > > No, for a for loop > > for ( [1]; [2]; [3] ) > > where [3] is ++j will increment j before use. However, in an > if-statement the complete statement [3] is evaluated before the test [2] > is done. If the compiler is smart it will produce the fastest binary > code regardless ++j or j++. However, if the [3] is more complicated like > a hypothetical i = ++j + k the compiler will most probably generate > different binary code (compared to i = ++j + k). The last line should say ... different binary code (compared to i = j++ + k). Sorry for the typo. Jari |
From: Gene B. <ge...@de...> - 2011-07-28 13:22:29
|
On Thu, 28 Jul 2011, Jari Häkkinen wrote: >>> Are you sure about that? I just tried it with a little example and at least >>> gcc compiles both variants to the exact same assembly code. Tried it with and >>> without -O2. >>> >> That would freak me out. Doesn't "++j" mean "increment j, then test" >> whereas "j++" means "test j, then increment"? > > No, for a for loop > > for ( [1]; [2]; [3] ) > > where [3] is ++j will increment j before use. However, in an > if-statement the complete statement [3] is evaluated before the test [2] > is done. If the compiler is smart it will produce the fastest binary > code regardless ++j or j++. However, if the [3] is more complicated like > a hypothetical i = ++j + k the compiler will most probably generate > different binary code (compared to i = ++j + k). Right, but j++ will increment _after_ it's used, correct? So how could ++j vs j++ generate the same assembly code and be correct? g. -- Proud owner of F-15C 80-0007 http://www.f15sim.com - The only one of its kind. http://www.simpits.org/geneb - The Me-109F/X Project Some people collect things for a hobby. Geeks collect hobbies. ScarletDME - The red hot Data Management Environment A Multi-Value database for the masses, not the classes. http://www.scarletdme.org - Get it _today_! Political correctness is a doctrine, fostered by a delusional, illogical minority, and rabidly promoted by an unscrupulous mainstream media, which holds forth the proposition that it is entirely possible to pick up a turd by the clean end. |
From: Jari H. <ja...@fl...> - 2011-07-28 13:48:01
|
On 2011-07-28 15.22, Gene Buckle wrote: > On Thu, 28 Jul 2011, Jari Häkkinen wrote: > >>>> Are you sure about that? I just tried it with a little example and >>>> at least >>>> gcc compiles both variants to the exact same assembly code. Tried it >>>> with and >>>> without -O2. >>>> >>> That would freak me out. Doesn't "++j" mean "increment j, then test" >>> whereas "j++" means "test j, then increment"? >> >> No, for a for loop >> >> for ( [1]; [2]; [3] ) >> >> where [3] is ++j will increment j before use. However, in an >> if-statement the complete statement [3] is evaluated before the test [2] >> is done. If the compiler is smart it will produce the fastest binary >> code regardless ++j or j++. However, if the [3] is more complicated like >> a hypothetical i = ++j + k the compiler will most probably generate >> different binary code (compared to i = ++j + k). > > Right, but j++ will increment _after_ it's used, correct? So how could > ++j vs j++ generate the same assembly code and be correct? Yes, j++ will increment j after use. The same assembly code in this case is probably because gcc makes optimizations for simple statements like j++; or ++j; These are straight forward for the compiler to perform optimizations for (at least for standard types like doubles, integers, chars etc). For general classes it is probably not legal for the compiler to make similar assumptions when optimizing since the operators ++C and C++ can in principle behave completely differently/unexpectedly ... we are in the mercy of the programmer here. Jari |
From: Stefan S. <ni...@de...> - 2011-07-28 13:49:47
|
On Thursday 28 July 2011 06:22:10 Gene Buckle wrote: > On Thu, 28 Jul 2011, Jari Häkkinen wrote: > >>> Are you sure about that? I just tried it with a little example and > >>> at least gcc compiles both variants to the exact same assembly > >>> code. Tried it with and without -O2. > >> > >> That would freak me out. Doesn't "++j" mean "increment j, then test" > >> whereas "j++" means "test j, then increment"? > > > > No, for a for loop > > > > for ( [1]; [2]; [3] ) > > > > where [3] is ++j will increment j before use. However, in an > > if-statement the complete statement [3] is evaluated before the test [2] > > is done. If the compiler is smart it will produce the fastest binary > > code regardless ++j or j++. However, if the [3] is more complicated like > > a hypothetical i = ++j + k the compiler will most probably generate > > different binary code (compared to i = ++j + k). > > Right, but j++ will increment _after_ it's used, correct? So how could > ++j vs j++ generate the same assembly code and be correct? Because the for loop is a case where it simply doesn't matter. Translate the for to a while: int i = 0; while (i < 10) i++; is just the same as: int i = 0; while (i < 10) ++i; because the value of the incrementing expression is not used anywhere. The compiler recognizes this as one of the simplest cases of optimization. Stefan |
From: ThorstenB <br...@gm...> - 2011-07-29 17:21:53
|
On 28.07.2011 00:30, Stuart Buchanan wrote: > On my machine I don't see any performance impact, despite the fact > that more trees are displayed. I'd appreciate it if those with more > graphics-constrained systems than my own would test this and let me > know if they think the frame-rate hit is acceptable given the > improvements in apparent tree coverage. Not sure if trees were also affected, but I remember a very ugly issue from last year. OSG is quick in sharing a single object multiple times into the a scenery ( O(1) ), but removing such a shared object is very expensive. OSG uses a single thread for loading and removing. Dropping a scenery area with an object shared a few thousand times only took a fraction of a second. But when the number of shares increased even further, then suddenly the OSG "database" thread blocked for minutes - while no new scenery could be loaded. O(n^2) bites you eventually. That's why we reduced or got rid of cattle, horse, ... objects scattered across the scenery. I'm not sure if increasing the tree count could trigger the same problem. And no idea if OSG3 has improved on this issue. The problem is only observed after the tile cache is full, so scenery tiles need to be dropped from memory for the first time. A good way for testing is to take the UFO and race at maximum speed across the scenery. Make sure scenery tiles keep continually loading and appearing at the same speed - so that even after a few minutes of flying, loading doesn't get blocked and you're flying into the void. cheers, Thorsten |
From: Stuart B. <stu...@gm...> - 2011-07-29 17:39:20
|
On Fri, Jul 29, 2011 at 6:21 PM, ThorstenB wrote: > On 28.07.2011 00:30, Stuart Buchanan wrote: >> On my machine I don't see any performance impact, despite the fact >> that more trees are displayed. I'd appreciate it if those with more >> graphics-constrained systems than my own would test this and let me >> know if they think the frame-rate hit is acceptable given the >> improvements in apparent tree coverage. > > Not sure if trees were also affected, but I remember a very ugly issue > from last year. OSG is quick in sharing a single object multiple times > into the a scenery ( O(1) ), but removing such a shared object is very > expensive. OSG uses a single thread for loading and removing. Dropping a > scenery area with an object shared a few thousand times only took a > fraction of a second. But when the number of shares increased even > further, then suddenly the OSG "database" thread blocked for minutes - > while no new scenery could be loaded. O(n^2) bites you eventually. > That's why we reduced or got rid of cattle, horse, ... objects scattered > across the scenery. > I'm not sure if increasing the tree count could trigger the same > problem. And no idea if OSG3 has improved on this issue. > > The problem is only observed after the tile cache is full, so scenery > tiles need to be dropped from memory for the first time. > A good way for testing is to take the UFO and race at maximum speed > across the scenery. Make sure scenery tiles keep continually loading and > appearing at the same speed - so that even after a few minutes of > flying, loading doesn't get blocked and you're flying into the void. Good to know. However, I don't think my change will have affected this. While the number of trees displayed is increased, the total number of trees in the scenery is unaffected, it's just that more of those trees are visible at any given time. I'm also not sure if the tree model is shared in this way. It's not really a model in the .ac sense anyway. -Stuart |
From: Torsten D. <To...@t3...> - 2011-07-29 20:06:36
|
> However, I don't think my change will have affected this. > > While the number of trees displayed is increased, the total number > of trees in the scenery is unaffected, it's just that more of those > trees are visible at any given time. > > I'm also not sure if the tree model is shared in this way. It's not > really a model > in the .ac sense anyway. I just merged it into next. Let's give it a try before the leaves begin to fall ;-) Torsten |
From: ThorstenB <br...@gm...> - 2011-07-31 19:52:39
|
On 28.07.2011 00:30, Stuart Buchanan wrote: > The patch does the following: > 1) Fixes the longstanding bug where the first set of tree definitions > in a tile were used for all forests within that tile. > 2) Re-introduce a feature to fade in trees by distance And, after doing a few circles at LOWI, the new forests really prove to be a very nice improvement! cheers, Thorsten |