Skip to content

Commit 690c499

Browse files
CrissplCrisspl
committed
Fixed bug 1
1 parent 24e82c4 commit 690c499

File tree

2 files changed

+14
-27
lines changed

2 files changed

+14
-27
lines changed

‎source/Nabla/COpenGLDriver.cpp

Lines changed: 10 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2002,13 +2002,14 @@ void COpenGLDriver::drawIndexedIndirect(const asset::SBufferBinding<IGPUBuffer>
20022002
extGlMultiDrawElementsIndirect(primType,indexSize,(void*)offset,maxCount,stride);
20032003
}
20042004

2005-
void COpenGLDriver::SAuxContext::flushState_descriptors(E_PIPELINE_BIND_POINT _pbp, const COpenGLPipelineLayout* _currentLayout, const COpenGLPipelineLayout* _prevLayout)
2005+
void COpenGLDriver::SAuxContext::flushState_descriptors(E_PIPELINE_BIND_POINT _pbp, const COpenGLPipelineLayout* _currentLayout)
20062006
{
2007+
const COpenGLPipelineLayout* prevLayout = effectivelyBoundDescriptors.layout.get();
20072008
//bind new descriptor sets
20082009
int32_t compatibilityLimit = 0u;
2009-
if (_prevLayout && _currentLayout)
2010-
compatibilityLimit = _prevLayout->isCompatibleUpToSet(IGPUPipelineLayout::DESCRIPTOR_SET_COUNT-1u, _currentLayout)+1u;
2011-
if (!_prevLayout && !_currentLayout)
2010+
if (prevLayout && _currentLayout)
2011+
compatibilityLimit = prevLayout->isCompatibleUpToSet(IGPUPipelineLayout::DESCRIPTOR_SET_COUNT-1u, _currentLayout)+1u;
2012+
if (!prevLayout && !_currentLayout)
20122013
compatibilityLimit = static_cast<int32_t>(IGPUPipelineLayout::DESCRIPTOR_SET_COUNT);
20132014

20142015
int64_t newUboCount = 0u, newSsboCount = 0u, newTexCount = 0u, newImgCount = 0u;
@@ -2035,23 +2036,13 @@ count = (first_count.resname.count - std::max(0, static_cast<int32_t>(first_coun
20352036
}
20362037

20372038
//if prev and curr pipeline layouts are compatible for set N, currState.set[N]==nextState.set[N] and the sets were bound with same dynamic offsets, then binding set N would be redundant
2038-
/*
2039-
// @Crisspl this is BUGGY.
2040-
// Imagine I have desc sets A, B, C
2041-
// I do some compute work while binding a pipeline layout with {A,B,C,nullptr}
2042-
// then I do some graphics work while binding a pipeline layout with {nullptr,B,nullptr}, I only ever use one pipeline
2043-
// when I do the graphics flush, the bindings will not be updated because prevLayout and currentLayout come from graphics (and are the same)
2044-
// AND the effectivelyBoundDescriptor matches with the only descriptor of the graphics bind point (both are B) this leads to problems
2045-
// you need to detect if switching effective pipelines (graphics to compute and back) will cause offsets to shift
2046-
// my suggestion is to track `current.effectivelyBoundDescriptors` and `next.effectivelyBoundDescriptors` then work from that instead of compare the next state for a pipeline with "previous" effectivelyBoundDescriptors
20472039
if ((i < compatibilityLimit) &&
20482040
(effectivelyBoundDescriptors.descSets[i].set == nextState.descriptorsParams[_pbp].descSets[i].set) &&
20492041
(effectivelyBoundDescriptors.descSets[i].dynamicOffsets == nextState.descriptorsParams[_pbp].descSets[i].dynamicOffsets)
20502042
)
20512043
{
20522044
continue;
20532045
}
2054-
*/
20552046

20562047
const auto multibind_params = nextState.descriptorsParams[_pbp].descSets[i].set ?
20572048
nextState.descriptorsParams[_pbp].descSets[i].set->getMultibindParams() :
@@ -2121,10 +2112,10 @@ count = (first_count.resname.count - std::max(0, static_cast<int32_t>(first_coun
21212112
}
21222113

21232114
//unbind previous descriptors if needed (if bindings not replaced by new multibind calls)
2124-
if (_prevLayout)//if previous pipeline was nullptr, then no descriptors were bound
2115+
if (prevLayout)//if previous pipeline was nullptr, then no descriptors were bound
21252116
{
21262117
int64_t prevUboCount = 0u, prevSsboCount = 0u, prevTexCount = 0u, prevImgCount = 0u;
2127-
const auto& first_count = _prevLayout->getMultibindParamsForDescSet(video::IGPUPipelineLayout::DESCRIPTOR_SET_COUNT - 1u);
2118+
const auto& first_count = prevLayout->getMultibindParamsForDescSet(video::IGPUPipelineLayout::DESCRIPTOR_SET_COUNT - 1u);
21282119

21292120
prevUboCount = first_count.ubos.first + first_count.ubos.count;
21302121
prevSsboCount = first_count.ssbos.first + first_count.ssbos.count;
@@ -2145,6 +2136,7 @@ count = (first_count.resname.count - std::max(0, static_cast<int32_t>(first_coun
21452136
}
21462137

21472138
//update state in state tracker
2139+
effectivelyBoundDescriptors.layout = core::smart_refctd_ptr<const COpenGLPipelineLayout>(_currentLayout);
21482140
for (uint32_t i = 0u; i < video::IGPUPipelineLayout::DESCRIPTOR_SET_COUNT; ++i)
21492141
{
21502142
currentState.descriptorsParams[_pbp].descSets[i] = nextState.descriptorsParams[_pbp].descSets[i];
@@ -2154,10 +2146,6 @@ count = (first_count.resname.count - std::max(0, static_cast<int32_t>(first_coun
21542146

21552147
void COpenGLDriver::SAuxContext::flushStateGraphics(uint32_t stateBits)
21562148
{
2157-
const COpenGLPipelineLayout* prevLayout = nullptr;
2158-
if ((stateBits & GSB_DESCRIPTOR_SETS) && currentState.pipeline.graphics.pipeline)
2159-
prevLayout = static_cast<const COpenGLPipelineLayout*>(currentState.pipeline.graphics.pipeline->getLayout());
2160-
21612149
if (stateBits & GSB_PIPELINE)
21622150
{
21632151
if (nextState.pipeline.graphics.pipeline != currentState.pipeline.graphics.pipeline)
@@ -2357,7 +2345,7 @@ void COpenGLDriver::SAuxContext::flushStateGraphics(uint32_t stateBits)
23572345
if (stateBits & GSB_DESCRIPTOR_SETS)
23582346
{
23592347
const COpenGLPipelineLayout* currLayout = static_cast<const COpenGLPipelineLayout*>(currentState.pipeline.graphics.pipeline->getLayout());
2360-
flushState_descriptors(EPBP_GRAPHICS, currLayout, prevLayout);
2348+
flushState_descriptors(EPBP_GRAPHICS, currLayout);
23612349
}
23622350
if ((stateBits & GSB_VAO_AND_VERTEX_INPUT) && currentState.pipeline.graphics.pipeline)
23632351
{
@@ -2557,10 +2545,6 @@ void COpenGLDriver::SAuxContext::flushStateGraphics(uint32_t stateBits)
25572545

25582546
void COpenGLDriver::SAuxContext::flushStateCompute(uint32_t stateBits)
25592547
{
2560-
const COpenGLPipelineLayout* prevLayout = nullptr;
2561-
if ((stateBits & GSB_DESCRIPTOR_SETS) && currentState.pipeline.compute.pipeline)
2562-
prevLayout = static_cast<const COpenGLPipelineLayout*>(currentState.pipeline.compute.pipeline->getLayout());
2563-
25642548
if (stateBits & GSB_PIPELINE)
25652549
{
25662550
if (nextState.pipeline.compute.usedShader != currentState.pipeline.compute.usedShader)
@@ -2591,7 +2575,7 @@ void COpenGLDriver::SAuxContext::flushStateCompute(uint32_t stateBits)
25912575
if (stateBits & GSB_DESCRIPTOR_SETS)
25922576
{
25932577
const COpenGLPipelineLayout* currLayout = static_cast<const COpenGLPipelineLayout*>(currentState.pipeline.compute.pipeline->getLayout());
2594-
flushState_descriptors(EPBP_COMPUTE, currLayout, prevLayout);
2578+
flushState_descriptors(EPBP_COMPUTE, currLayout);
25952579
}
25962580
}
25972581

‎source/Nabla/COpenGLDriver.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -856,14 +856,17 @@ class COpenGLDriver final : public CNullDriver, public COpenGLExtensionHandler
856856
VAOMap.reserve(maxVAOCacheSize);
857857
}
858858

859-
void flushState_descriptors(E_PIPELINE_BIND_POINT _pbp, const COpenGLPipelineLayout* _currentLayout, const COpenGLPipelineLayout* _prevLayout);
859+
void flushState_descriptors(E_PIPELINE_BIND_POINT _pbp, const COpenGLPipelineLayout* _currentLayout);
860860
void flushStateGraphics(uint32_t stateBits);
861861
void flushStateCompute(uint32_t stateBits);
862862

863863
SOpenGLState currentState;
864864
SOpenGLState nextState;
865+
// represents descriptors currently flushed into GL state,
866+
// layout is needed to disambiguate descriptor sets due to translation into OpenGL descriptor indices
865867
struct {
866868
SOpenGLState::SDescSetBnd descSets[IGPUPipelineLayout::DESCRIPTOR_SET_COUNT];
869+
core::smart_refctd_ptr<const COpenGLPipelineLayout> layout;
867870
} effectivelyBoundDescriptors;
868871

869872
//push constants are tracked outside of next/currentState because there can be multiple pushConstants() calls and each of them kinda depends on the pervious one (layout compatibility)

0 commit comments

Comments
 (0)