Skip to content
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: Support having transparent ManagedSurface #2789

Merged
merged 1 commit into from Feb 22, 2021

Conversation

@criezy
Copy link
Member

@criezy criezy commented Feb 21, 2021

A change made yesterday forces ManagedSurface to be fully opaque. This may be an issue for some engines.
This commit revert this and also adds proper handling of the surface alpha when blitting pixels.

graphics/managed_surface.cpp Outdated Show resolved Hide resolved
graphics/managed_surface.cpp Outdated Show resolved Hide resolved
@criezy criezy force-pushed the criezy:transparent-managed_surface branch from 1207471 to 27801dd Feb 21, 2021
@OMGPizzaGuy
Copy link
Member

@OMGPizzaGuy OMGPizzaGuy commented Feb 21, 2021

ManagedSurface::blitFromInner also blends, but it I think it's safe to assume it should always end with 0xFF as alpha there.

@criezy
Copy link
Member Author

@criezy criezy commented Feb 21, 2021

ManagedSurface::blitFromInner also blends, but it I think it's safe to assume it should always end with 0xFF as alpha there.

I think it might be better to also handle the destination transparency there. I will have a look.

@criezy criezy force-pushed the criezy:transparent-managed_surface branch from 27801dd to 9fbffcc Feb 21, 2021
@criezy
Copy link
Member Author

@criezy criezy commented Feb 21, 2021

I had a quick look at ManagedSurface::blitFromInner and at the very least it is not consistent between the case where the source and destination formats are the same and the generic case. In the former case it just copies the source onto the destination with no blending, which is probably incorrect if the source is partially transparent. So I think I will need to do some changes here anyway.

Edit: And the blending is also wrong. It doesn't blend with the destination pixel, but with the previous blended pixel...

@OMGPizzaGuy OMGPizzaGuy self-requested a review Feb 21, 2021
@criezy criezy force-pushed the criezy:transparent-managed_surface branch from 9fbffcc to e539e8d Feb 21, 2021
@criezy
Copy link
Member Author

@criezy criezy commented Feb 21, 2021

I have now fixed the obviously wrong alpha blending in ManagedSurface::blitFromInner and decided to add support for transparent destination there.

I have played through a few games in three different engines that use ManagedSurface (AGS, Glk, and Cryomni3d) and did not see any regression.

@criezy criezy force-pushed the criezy:transparent-managed_surface branch from e539e8d to d270c5c Feb 21, 2021
@criezy
Copy link
Member Author

@criezy criezy commented Feb 22, 2021

Thank you for the review and for pointing out my stupid mistakes.
I will merge this now and cross my fingers that it does not introduce regressions..

@criezy criezy merged commit f12813c into scummvm:master Feb 22, 2021
3 checks passed
3 checks passed
Codacy Static Code Analysis Codacy Static Code Analysis
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants