Opened 11 years ago
Closed 10 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)
Change History (14)
comment:2 by , 11 years ago
Cc: | added |
---|
comment:3 by , 11 years ago
Milestone: | Alpha 13 → Alpha 14 |
---|
by , 11 years ago
Attachment: | ModelCache.patch added |
---|
comment:4 by , 11 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]".
comment:5 by , 11 years ago
Can you be clearer about what this latest patch is accomplishing? Is it just a cleanup of myconid's code?
comment:7 by , 11 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 , 11 years ago
Keywords: | review removed |
---|
comment:9 by , 11 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 , 11 years ago
Attachment: | ModelCache.2.patch added |
---|
comment:10 by , 11 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 , 11 years ago
Milestone: | Alpha 14 → Alpha 15 |
---|
comment:12 by , 10 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Rendered useless by Philip's commit in [13911].
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 there was a definition for "materialBuckets[key" so if you have an idea, do tell me.