Effects refactoring: loaded effect#4426
Conversation
| m_pManifest = pManifest; | ||
|
|
||
| if (!pManifest || !pEffectPreset) { | ||
| // No new effect to load; just unload the old effect and return. | ||
| emit effectChanged(); | ||
| return; | ||
| } | ||
|
|
||
| m_pManifest = pManifest; |
There was a problem hiding this comment.
This looks wrong. m_pManifest being nullptr indicates that the EffectSlot is empty, which needs to happen before this function returns.
There was a problem hiding this comment.
m_pManifest is always cleared at this point.
There was a problem hiding this comment.
No, if loading nullptr this function would return before setting m_pManifest to nullptr.
There was a problem hiding this comment.
It is already cleared via unloadEffect();
There was a problem hiding this comment.
Okay, but that's not obvious. There's no benefit to this change; it only makes the code harder to read. Rebase to remove this commit.
There was a problem hiding this comment.
Well then a DEBUG_ASSERT must be added instead of implicitly assuming unloadEffect does what the caller expects.
There was a problem hiding this comment.
Daniel just pointed the actual bug out that I didn't even spot after telling me there is one. The problem occurs when calling loadEffectInner with pManifest being valid but pEffectPreset being nullptr. The slot would result in being unloaded, but having a valid m_pManifest. From what I can tell, this violates an invariant.
The proposed change would eliminate the issue.
There was a problem hiding this comment.
Okay. The DEBUG_ASSERT is still needed.
There was a problem hiding this comment.
So have we found the consensus of sticking with the reordering but adding a debug assert between unloadEffect and return?
There was a problem hiding this comment.
Also I think a debug assertion should be added checking that pEffectPreset is not nullptr when pManifest is valid.
|
I have cherry picked these commits to my branch except for 8c77516. |
|
Now that I have started to prepare the effects refactoring branch. I expect the swapped roles in a normal review process. This will also give others the chance to give comments and merge in case we are at a different opinion. |
|
Okay, sorry, I was not clear what workflow you were expecting. Rebase to remove 8c77516 and we can merge this. |
daschuer
force-pushed
the
effects_refactoring
branch
from
4141b2d to
02e7b6a
Compare
|
Done. |
3 participants
This fixes some review issues #2618 around EffectSlot and loaded_effect.
See commits.