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

GRAPHICS: Consider flip mode when handling off-screen clipping #979

Merged
merged 1 commit into from Aug 11, 2017

Conversation

Projects
None yet
3 participants
@yinsimei
Contributor

yinsimei commented Aug 3, 2017

Hi, I meet a bug concerning transparent surface when importing the sludge engine, which is, when a horizontally flipped sprite leaves the scene from the left side, it begins to disappear from the right side. So I guess maybe we need to take the flip mode in to account when handling off-screen clipping in transparent_surface.cpp and it works for Sludge games.
But I'm not sure if this is the right level to fix the bug as this part of code has existed for a long time without anyone else noticing this need, so I made a PR to discuss the problem.

@yinsimei yinsimei requested review from sev- and tobiatesan Aug 3, 2017

@tobiatesan

Thanks for addressing this with a PR.

  • I believe, all things considered, this can be the appropriate place to add this behaviour.

  • My brain is not the most sound of static analyzers, but at a glance the changes appear reasonable (as in, correct) to me.

  • At a glance I don't see an obvious performance cost either. Good.

  • transparent_surface.h is used in sword25, wintermute, fullpipe, wage and director.
    Wintermute seems unaffected by the change, ie. it doesn't apparently break anything in there. Also good.

  • flipping seems to be used exclusively in sword25 and fullpipe.
    I don't have a copy to test if it works in there. @sev- appears to be the main author of fullpipe and one of the largest contributors to sword25 along with @bluegr, so I'd ask them if this matches the semantics they have/had in mind just to be sure. Skimming the code in engines/sword25 and engines/fullpipe I don't see any obvious inconsistencies.

  • @csnover and the SCI gang might also be interested in these changes.

@sev-

This comment has been minimized.

Show comment
Hide comment
@sev-

sev- Aug 11, 2017

Member

I tested both full pipe and sword25, both seem to not have any introduced glitches with this patch.

So, I am merging it.

Member

sev- commented Aug 11, 2017

I tested both full pipe and sword25, both seem to not have any introduced glitches with this patch.

So, I am merging it.

@sev- sev- merged commit 7d60ccc into scummvm:master Aug 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment