Opened 10 years ago

Closed 9 years ago

#1859 closed defect (wontfix)

[PATCH] Fix the caching of texture bindings in ModelRenderer.cpp

Reported by: wraitii Owned by: wraitii
Priority: Should Have Milestone: Alpha 15
Component: Core engine Keywords: patch
Cc: myconid Patch:

Description

Was looking at ModelRenderer.cpp and noticed that contrarily to what was indicated, Myconid was recreating the cache for texture bindings for every model, which is a notable slow-down.

I've changed it slightly to avoid the reallocation. There was a GL Error with SpecTex, apparently (I think because of how it's filtered) we can't rely on the names to make sure we don't have to rebind. Furthermore it seems pretty dubious how those arrays are cleaned, I'm not sure that works. Anyway, please test and report for GL Errors at different levels of shader quality. (noticed up to 10 ms speedup on Gambia River when zooming out).

Attachments (2)

ModelCache.patch (4.1 KB ) - added by wraitii 10 years ago.
ModelCache.2.patch (5.6 KB ) - added by wraitii 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 by wraitii, 10 years ago

Uploaded a new patch... So basically I've given a good look at the code and it seems like Myconid was actually doing the caching to some extent, only he did it in a way that I think was pretty convoluted. The above patch does basically the same thing in a simpler to read way. It caches only the first sampler, not the other ones (which Myconid's did) but there is less overhead for other things so it balances that. The lag caused by binding texture is completely irrelevant anyway as it's incredibly small compared to the cost of actually rendering the models ( < 1%). It could however be updated to cache all textures but I'm not sure how interesting that would be since sorting is only done against the first texture (could speed up trees, perhaps).

Not too sure why 2 steps were necessary to push models in "materialBuckets[key]" so if you have an idea, do tell me.

Last edited 10 years ago by wraitii (previous) (diff)

comment:2 by historic_bruno, 10 years ago

Cc: myconid added

comment:3 by Kieran P, 10 years ago

Milestone: Alpha 13Alpha 14

by wraitii, 10 years ago

Attachment: ModelCache.patch added

comment:4 by wraitii, 10 years ago

Okay, after some work on the renderer I thinkI actually know what I'm talking about now... So basically Myconid's stuff worked but it was not really working as advertised, nor as efficiently. The problem is that when we change the shader, we need to reset the caches with default values (they actually need default values or they will cause GLerrors at the first comparison). Myconid avoided that by checking the size, which was always 0 the first time since the vectors were cleared, so he did reallocate. In the chance the samplers num changed, though, he reallocated things, so there were a few small memory reallocations here and there. My patch is basically a modification of his code to clean a few things up, and make it slightly easier to read.

Still not sure why 2 steps were wanted to push models in "materialBuckets[key]".

Last edited 10 years ago by wraitii (previous) (diff)

comment:5 by historic_bruno, 10 years ago

Can you be clearer about what this latest patch is accomplishing? Is it just a cleanup of myconid's code?

comment:6 by wraitii, 10 years ago

Basically, yes.

comment:7 by Jorma Rebane, 10 years ago

Hello, wraitii. I was assigned by Michael to do a quick code-review of your patch. :)

At patch line 592: when doing the initial init of the vectors "currentTexs", "texBindings" and "texBindingNames", you can just call 'vector::resize', which does the equivalent. The C++ standard guarantees calling the default value initializers:

currentTexs.resize(16); // all new pointers get NULL
texBindings.resize(16); // all new bindings get CShaderProgram::Binding()
texBindingNames.resize(16); // all new names get CStrItern()

For patch line 635: using the loop makes sense, since you avoid orphaning entire vector range with clear (calls dtors on vector range). However, you should loop by using 'vector::size()', even though it works in that situation because of reserve(). If it would happen that size() != capacity(), you would overrun the vector bounds.

At patch line 663: you should still check by the 'vector::size()', since in general size() != capacity().

At source line 646: good job removing unnecessary std::fill calls!

At patch line 678: you could use != operator for clarity. Otherwise though, great job spotting that little bug in the binding system! ;)

If you can fix these little discrepancies I believe we should add your patch to the current build. Also, sorry for being too detailed; the patch was tiny enough, so it was easy to give you a full review - hopefully you like thoroughness :)

comment:8 by historic_bruno, 10 years ago

Keywords: review removed

comment:9 by wraitii, 10 years ago

Further comments (replying to each point)

  • Allright, will change.
  • True, hadn't thought of that.
  • I'm not too sure actually, because as I understood it we only wanted to make sure the vector had enough space available, not necessarily that it was the right size, but that might be a mistake from me.
  • Actually, I can't, because it doesn't exist (I think. I recall my compiler throwing errors.)

Thanks for the thoroughness though, on such a matter I'd rather have someone knowing his code to review :)

by wraitii, 10 years ago

Attachment: ModelCache.2.patch added

comment:10 by wraitii, 10 years ago

New patch with a set maximum size of 4. It's currently explicitely written, but that'll change when RedFox adds a MAX_SAMPLER constant to CMaterial.

I didn't really know how to handle cases (there are none right now) where the samplers are above 4, so I put a branching there, but that probably should be removed since we're not ever using it and we'll probably never once Max_Sampler is implemented.

comment:11 by Kieran P, 9 years ago

Milestone: Alpha 14Alpha 15

comment:12 by wraitii, 9 years ago

Resolution: wontfix
Status: newclosed

Rendered useless by Philip's commit in [13911].

Note: See TracTickets for help on using tickets.