New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SpriteEffect not updating ScreenMap #14048

Merged
merged 2 commits into from Sep 24, 2017

Conversation

Projects
None yet
4 participants
@reaperrr
Contributor

reaperrr commented Sep 17, 2017

Fixes #14047.

As additional safe-guarding against zero-bounds, the adding/removing in ScreenMap was tweaked as well, now it only adds effects if W/H are not 0.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 17, 2017

Contributor

Updated.

Contributor

reaperrr commented Sep 17, 2017

Updated.

@reaperrr reaperrr requested review from pchote and RoosterDragon Sep 17, 2017

@reaperrr reaperrr added this to the Next + 1 milestone Sep 17, 2017

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Sep 19, 2017

Member

Why are their effects with bounds of zero in the first place? What use do they have? I know we had to exclude some actors with zero bounds because they're invisible markers for things, is that the case here as well?

If we do need the bounds check, the way it was applied previously for actors and such was further up the call stack:

if (!self.Bounds.Size.IsEmpty)
    self.World.ScreenMap.Add(self);

So I guess we would want similar checks for consistency, if we do need to add them.

Member

RoosterDragon commented Sep 19, 2017

Why are their effects with bounds of zero in the first place? What use do they have? I know we had to exclude some actors with zero bounds because they're invisible markers for things, is that the case here as well?

If we do need the bounds check, the way it was applied previously for actors and such was further up the call stack:

if (!self.Bounds.Size.IsEmpty)
    self.World.ScreenMap.Add(self);

So I guess we would want similar checks for consistency, if we do need to add them.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 19, 2017

Member

For actors we have always used a fixed size regardless of their current rendered state, but for effects we are updating them every tick to reflect what is actually visible. The issue comes when the effect legitimately doesn't want to render something during one of the frames of its animation.

IMO handing this lower level in the stack is cleaner, and we will eventually need to do something similar for actors as part of the fix for bibs and shadows.

Member

pchote commented Sep 19, 2017

For actors we have always used a fixed size regardless of their current rendered state, but for effects we are updating them every tick to reflect what is actually visible. The issue comes when the effect legitimately doesn't want to render something during one of the frames of its animation.

IMO handing this lower level in the stack is cleaner, and we will eventually need to do something similar for actors as part of the fix for bibs and shadows.

Only add partitioned effects to ScreenMap if (current) bounds are valid
This serves to avoid adding effect where either width or height is 0.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 19, 2017

Contributor

Updated.

Contributor

reaperrr commented Sep 19, 2017

Updated.

@pchote

pchote approved these changes Sep 24, 2017

I don't have a repro case for the original issue so I can't verify that this is sufficient to fix the crash, but these will definitely help.

@pchote pchote merged commit 4ae92a5 into OpenRA:bleed Sep 24, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@reaperrr reaperrr deleted the reaperrr:fix-spriteeffect-crash branch Nov 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment