Patch review: ao/parallax/normal/specular/emissive mapping; windy trees; time manag…

[zoot]

ao/parallax/normal/specular/emissive mapping; windy trees; time manager; render queries; etc

https://github.com/m...c15520e21e72209

The community is invited to help review this patch. Discussion of the code is welcome.

To apply the patch for testing, use git cherry-pick or old-school patch.

Edited August 5, 2012 by zoot

[zoot]

I've been poring over the time manager component. Thinking out loud (doesn't imply anything is wrong with the code):

The patch instantiates in the CRenderer class a "time manager" object which can be accessed through CRenderer::GetTimeManager().

Every time CGame::Update() is called, g_Renderer.GetTimeManager().Update(deltaSimTime) is called. Not completely sure what that means, but I assume it sends the time elapsed since last frame (the time 'delta') to the time manager on every frame update.

The time manager stores the delta in a private variable m_frameDelta and adds it to another private variable m_globalTime. The point of the 'global time' variable would seem to be to keep track of time elapsed since the beginning of the simulation (?). It can be accesed via CTimeManager::GetGlobalTime().

This global time seems to be sent to another new component ('render queries') here, presumably for being passed on to shaders.

I didn't come across any glaring errors in any of the above.

[zoot]

General note: the patch sets preferglsl=true by default, where I think the consensus is that it should remain false for now.

[zoot]

Let's see what this 'render queries' thing does.

One thing to note is that it adds a new <renderquery> element to the material XML, like here.

It also instantiates in the CMaterial class an object to hold 'render queries' which can be accessed through CMaterial::GetRenderQueries() here. New render queries can be added through CMaterial::AddRenderQuery() here. Each render query appears to be represented by a string key.

When the XML of a given material is parsed, the 'name' of each <renderquery> element is added as a key to the render queries object of the material here.

Currently, only one such render query 'name' is recognized, one called 'time'. If this name is added to the render queries object, it is added to the object's private array of render queries. Otherwise, it is discarded.

Note: There seems to be an superficial inconsistency with these 'name' strings sometimes referred to as 'name' and other times 'key'.

Note: There seems to be a small amount of code pertaining to render queries that has been commented out throughout the patch, like here and here. Should this be left in?

In ShaderModelRenderer::Render(), the array of render queries is iterated through here. If any 'time' queries are found, the global time is retrieved from the time manager described in the post above and passed to shaders as an OpenGL 'uniform' variable named 'time'.

I didn't come across any glaring errors in any of the above.

Edited August 5, 2012 by zoot

[zoot]

Next feature in line: quality levels.

This adds a quality setting to the config here. It also adds a constructor to CMaterialManager, which hardcodes the engine's default quality level to 5.0. The default is overridden by the manually specified quality setting, if one is present in the config. The quality level is then clamped to the range from 0.0 to 10.0.

Quality levels apparently supersede the old 'if' attribute of the 'alternative' element in material XML.

I'm not sure I understand this statement though:

if (!(!cond.empty() && preprocessor.TestConditional(cond)))

Would it not be slightly less confusing if written as:

if (cond.empty() || !preprocessor.TestConditional(cond)))

Anyhow, the point seems to be that if the old 'if' check fails, the engine will see if there is a 'quality' attribute instead. If there is no 'quality' attribute or if the 'quality' attribute is less than or equal to the quality level specified in the config, the engine will skip (?) loading the material alternative. (I would have thought it would be the reverse - if the 'quality' of a given material alternative exceeds the configured quality level, it should not be loaded?) Otherwise, the material alternative will be loaded as normal (?).

Edited August 5, 2012 by zoot

[myconid]

Just noticed this thread!

Ok, removed the commented stuff and changed the ugly conditional. So far so good!

Alternative materials are loaded instead of the current material, not in addition to it. The idea is that models always try to load the best-quality material available, and if the user's config settings aren't high enough, an alternative material is loaded that uses fewer effects (this goes on recursively until an acceptable quality level is reached, or no alternatives are available -- it's also possible to have several alternatives in a single material, as long as the quality values are in descending order). It could have been done the other way around, of course, by starting at the lowest-quality material and going upwards, though either way the idea is pretty much the same.

[zoot]

Alternative materials are loaded instead of the current material, not in addition to it. The idea is that models always try to load the best-quality material available, and if the user's config settings aren't high enough, an alternative material is loaded that uses fewer effects (this goes on recursively until an acceptable quality level is reached, or no alternatives are available -- it's also possible to have several alternatives in a single material, as long as the quality values are in descending order). It could have been done the other way around, of course, by starting at the lowest-quality material and going upwards, though either way the idea is pretty much the same.

Right, now I think I see it. As it iterates through the descending list of alternatives, it loads each one until reaching one that is below the quality setting? If this is the case, it does IMO seem a wee bit counter-intuitive that you might end up with a material whose quality is higher than your quality setting. It is in effect a minimum quality setting rather than a maximum quality setting?

In a sense you're right - why shouldn't it be a minimum quality setting? I guess my concern is that users (AFAIK) tend to use these types of controls to specify "maximum tolerable inefficiency/lag".

Maybe someone else can share their views on this. Or, if not, leave it as it is

[myconid]

Consider this example. Say the config setting is at 0 and the model definition points to Material 1.

Material 1:

<blah>
   <alternative (Material 2) quality=7/>
   ...
</blah>

If config < 7, jump to Material 2. Since config == 0, jump.

Material 2:

<blah>
   <alternative (Material 3) quality=3/>
   ...
</blah>

If config < 3, jump to Material 3. Since config == 0, jump.

Material 3:

<blah>
   ...
</blah>

There is no alternative, so this material is used iff config >= 0, so pick this one (this is the stopping condition for the recursion).

So, Materials 1 and 2 are ignored, and we load Material 3. If config was 4, it would pick Material 2. If config was 9, it would pick Material 1.

It's also possible to concentrate all the conditions in the same place to avoid the recursion, though obviously if a model starts off at Material 2 or 3, this system wouldn't work*.

Material 1:

<blah>
   <alternative (Material 3) quality=3/>
   <alternative (Material 2) quality=7/>
   ...
</blah>

This means if config < 3 jump to Material 3, else if config < 7 jump to Material 2, else pick Material 1 for anything >= 7.

(I guess this is ascending order, not descending... ). There's no need for "alternative" definitions in Materials 2 & 3, though they would help with this* problem, but then we're duplicating stuff.

[zoot]

Right, seems elegant enough. Probably what I was talking about ("maximum tolerable inefficiency") should be addressed with some other solution, like the ability to toggle individual effects on and off.

Edited August 6, 2012 by zoot

[Sonarpulse]

Maybe give the user a warning when a the lowest alternative is higher than their configuration asked for?

[zoot]

I must admit I'm a bit ambivalent about the whole notion of trying to map quality to an absolute, global scale. What if you have a well-performing effect called super mapping that looks excellent (quality=10) and a separate low-performing effect called meh mapping that doesn't look too good (quality=5) - if you want to improve performance of meh mapping, you can try and reduce the quality config setting (so as to select a better performing alternative), but then you also risk reducing the quality of super mapping, which isn't really necessary because it performs well already?

[myconid]

Then you'd use the "conditional_define" system I've added, which lets you set options for individual effects through the config.

[zoot]

I see. But doesn't conditional defines then make the quality level system obsolete? Can quality levels do anything that conditional defines cannot?

[myconid]

Generally speaking they kinda do, but there is one important difference.

Conditional defines are executed dynamically and (in the current implementation at least) re-evaluated by the renderer on each frame. The overhead is pretty minuscule, of course, but it's still there. OTOH, quality settings are evaluated only once, statically, as the materials are being loaded.

[zoot]

Would it be messy to make the conditional defines work for both cases?

[myconid]

Probably not. It's definitely something to think about in the future.

[zoot]

Conditional defines now on review.

It adds a new <conditional_define> element to material XML, like this. It also instantiates a new object in the CMaterial class for holding conditional defines here. It can be accessed through the GetConditionalDefines() method here and new conditional defines can be added to the object with the AddConditionalDefine() method here.

The new element is parsed here. The element takes a number of attributes, one of which is 'type'. The 'type' attribute specifies, in effect, a comparison operator for the conditional define.

Currently, only one such operator 'type' is recognized, namely 'draw_range'. The 'draw_range' operator loads two config values (a 'min' and a 'max' value) specified in the 'conf' attribute of the 'conditional_define' element. If no 'conf' attribute is present, it instead tries loading the 'min' and 'max' value from the element attributes. If the 'min' and 'max' values are greater than or equal to zero, they are added to the material as a 'define' for shaders.

When the 'type' attribute has been processed, the conditional define is then added to the material.

While rendering, the engine iterates through the conditional defines here. If a given conditional define is of the 'draw_range' type, the engine calculates the distance from the model being drawn to the camera. If the distance is within the 'min' and 'max' ranges specified for the conditional define, the 'value' attribute of the conditional define is added as a define with the name given by the 'name' attribute.

I didn't come across any glaring errors in any of the above.