Skip to content

Commit

Permalink
[Bugfix] More correct way to fix camera glitching
Browse files Browse the repository at this point in the history
In commit 0aab576 we have attempts to fix camera glitching when one of effectors finishes working. The root of glitches is effector's deletion in CCameraManager::ProcessCameraEffector. Cycle in CCameraManager::UpdateCamEffectors uses reverse iterator and doesn't expect that entry in the list would be removed. Due to specific behavior of reverse iterator it really points to the next element relative to element which it dereferenced to. So, when ProcessCameraEffector deletes currently dereferenced element, iterator automatically starts point to element before deleted one. But the UpdateCamEffectors doesn't know about it, and cycle changes iterator on next iteration. So, processing of one camera effect would be skipped in this moment.
Changes in 0aab576 don't seems to be the best solution, because
1) vector is less suitable for situations where we are often want to add or remove random elements
2) Deletion of current effector in ProcessCameraEffector seems to be not safe. For example, we could have CCameraManager's childs with overridden ProcessCameraEffector method. They could be deadly surprised if pointer to current effector would start pointing to non-existent element.
So, this commit:
1) Reverts changes from 0aab576
2) Removes effector's deletion from CCameraManager::ProcessCameraEffector; now the method returns false if it should be deleted, or true otherwise
3) Adds code for removing 'old' effectors to UpdateCamEffectors method.
  • Loading branch information
gunslingermod authored and Xottab-DUTY committed Aug 14, 2017
1 parent 42ac33e commit e270edf
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 15 deletions.
38 changes: 24 additions & 14 deletions src/xrEngine/CameraManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ void CCameraManager::UpdateDeffered()
RemoveCamEffector((*it)->eType);

if ((*it)->AbsolutePositioning())
m_EffectorsCam.insert(m_EffectorsCam.begin(), *it);
m_EffectorsCam.push_front(*it);
else
m_EffectorsCam.push_back(*it);
}
Expand Down Expand Up @@ -222,23 +222,19 @@ void CCameraManager::Update(const Fvector& P, const Fvector& D, const Fvector& N

bool CCameraManager::ProcessCameraEffector(CEffectorCam* eff)
{
// Do NOT delete effector here! It's unsafe because:
// 1. Leads to failed iterators in UpdateCamEffectors
// 2. Child classes with overrided ProcessCameraEffector would be surprised if eff becames invalid pointer
// The best way - return 'false' when the effector should be deleted, and delete it in ProcessCameraEffector

bool res = false;
if (eff->Valid() && eff->ProcessCam(m_cam_info))
{
res = true;
}
else
else if (eff->AllowProcessingIfInvalid())
{
if (eff->AllowProcessingIfInvalid())
{
eff->ProcessIfInvalid(m_cam_info);
res = true;
}

EffectorCamVec::iterator it = std::find(m_EffectorsCam.begin(), m_EffectorsCam.end(), eff);

m_EffectorsCam.erase(it);
OnEffectorReleased(eff);
eff->ProcessIfInvalid(m_cam_info);
}
return res;
}
Expand All @@ -247,8 +243,22 @@ void CCameraManager::UpdateCamEffectors()
{
if (m_EffectorsCam.empty())
return;
for (int i = m_EffectorsCam.size() - 1; i >= 0; --i)
ProcessCameraEffector(m_EffectorsCam[i]);

auto r_it = m_EffectorsCam.rbegin();
while (r_it != m_EffectorsCam.rend())
{
if (ProcessCameraEffector(*r_it))
++r_it;
else
{
// Dereferencing reverse iterator returns previous element of the list, r_it.base() returns current element
// So, we should use base()-1 iterator to delete just processed element. 'Previous' element would be
// automatically changed after deletion, so r_it would dereferencing to another value, no need to change it
OnEffectorReleased(*r_it);
auto r_to_del = r_it.base();
m_EffectorsCam.erase(--r_to_del);
}
}

m_cam_info.d.normalize();
m_cam_info.n.normalize();
Expand Down
2 changes: 1 addition & 1 deletion src/xrEngine/CameraManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#include "CameraDefs.h"
#include "xrCore/PostProcess/PPInfo.hpp"

using EffectorCamVec = xr_vector<CEffectorCam*>;
using EffectorCamVec = xr_list<CEffectorCam*>;
using EffectorPPVec = xr_vector<CEffectorPP*>;

#define effCustomEffectorStartID 10000
Expand Down

0 comments on commit e270edf

Please sign in to comment.