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

Support 32 bit RGBA sprite sequences #16885

Merged
merged 4 commits into from Jan 17, 2020
Merged

Support 32 bit RGBA sprite sequences #16885

merged 4 commits into from Jan 17, 2020

Conversation

@pchote
Copy link
Member

pchote commented Aug 5, 2019

Last year, #15194 unified Indexed vs RGBA sprites at the shader level and removed the performance cliff caused by switching between them. This PR takes another step on the slow-burning renderer rewrite by carrying the unification through the Sequence and SpriteLoader code.

Modders should not get too excited just yet: all the caveats of not having a palette still apply, so RGBA sprites won't yet work with player colours, target flashes, map lighting, alpha/shadows, and probably more. I expect that this won't be properly usable until we implement vertex-based color tinting/alpha, mask or chroma-key based player colors, and #3692.

The testcase demonstrates @mabulsoud's radar dome from #13245 ingame.

A more in-depth testcase is available on my d2k-r16 branch, which switches D2k to use the higher quality R16 sprites (which you will need to find and install manually).

@pchote pchote added this to the Next+1 milestone Aug 5, 2019
@pchote pchote force-pushed the pchote:rgba-sequences branch from f0a87c1 to 06947c8 Aug 5, 2019
@dragunoff

This comment has been minimized.

Copy link
Contributor

dragunoff commented Sep 3, 2019

I've been using this PR for a while and successfully loading 32-bit sprites but I noticed that R and B channels are swapped. If I for example pass this in my sprite's image data: (uint)Color.Red.ToArgb() it would render as blue.

@dragunoff

This comment has been minimized.

Copy link
Contributor

dragunoff commented Sep 3, 2019

Another weird thing I noticed is that some images are cut off (missing one row or column of pixels), the TD icon for example:
изображение

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Sep 3, 2019

Are you certain this is a regression from this PR? We had issues with such missing lines before as well.

@dragunoff

This comment has been minimized.

Copy link
Contributor

dragunoff commented Sep 3, 2019

Are you certain this is a regression from this PR? We had issues with such missing lines before as well.

No, I'm not sure about that. But I think it tends to happen with 32-bit sprites.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 4, 2019

@dragunoff can you provide your testcases?

@dragunoff

This comment has been minimized.

Copy link
Contributor

dragunoff commented Dec 8, 2019

@pchote Please have a look at dragunoff@8713123

I've added some simple color bars that render fine a single frame but are all jumbled up when sliced into frames. I have not tested the sprites in-game, only in the asset browser. I hope the metadata is correct. The TD logo also renders with the first row of the image missing (screenshot in an earlier message).

For reference, this is what I'm seeing:
macQ1nUrF8

@dragunoff

This comment has been minimized.

Copy link
Contributor

dragunoff commented Dec 18, 2019

The problem I illustrated above is actually caused by the wrong frame offset calculation that I already mentioned in #16885 (comment).

After fixing that the problem goes away and frames render correctly (see
dragunoff@ad7f468).

I will check again how sprites render on my downstream branch and if I am still able to reproduce the problem.

@dragunoff

This comment has been minimized.

Copy link
Contributor

dragunoff commented Dec 19, 2019

OK, this is happening only on Windows and only in the asset browser. It also affects 8-bit PNGs and my custom sprite loaders so it's probably a different issue altogether (something with the asset browser maybe?). Initially I thought it's an issue with 32-bit sprites or with my sprite loaders but after testing on an Ubuntu machine I found that everything looks fine both in-game and in the asset browser.

Here's what I see in the asset browser on Windows 7:
vQxkBqADr0

@pchote pchote force-pushed the pchote:rgba-sequences branch 2 times, most recently from ca9ef2f to 96f9a79 Dec 28, 2019
@pchote pchote removed the PR: Rebase me! label Dec 28, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 28, 2019

Rebased and fixed the frame offset issue. I still need to look into the reversed color channels and implement cursor support.

@pchote pchote force-pushed the pchote:rgba-sequences branch 2 times, most recently from afd4235 to efdfa2d Dec 29, 2019
@pchote pchote force-pushed the pchote:rgba-sequences branch from efdfa2d to 5ab5d85 Dec 29, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 29, 2019

Updated. The color channel issue was a silly oversight: OpenRA internally uses BGRA channel order, not RGBA. Renamed SpriteFrameType.RGBA to SpriteFrameType.BGRA to account for this.

The cursor changes turned into a bigger rework, so I will file these shortly as a followup PR.

Would you mind giving this another look @dragunoff?

@pchote pchote mentioned this pull request Dec 29, 2019
@pchote pchote force-pushed the pchote:rgba-sequences branch from ef353b0 to dbc9afb Jan 4, 2020
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 4, 2020

Rebased.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 16, 2020

I would really like to remove the requirement to restart after changing the "Increase Command Cursor Size" setting for #17345 (plus make it apply to the default cursor), but doing this cleanly requires merging HardwareCursor and SoftwareCursor, which requires this PR.

There should be nothing controversial, or even particularly complicated here. Can we please try and make some progress here?

Copy link
Contributor

reaperrr left a comment

Ingame looks good, didn't spot any code issues.

@pchote pchote force-pushed the pchote:rgba-sequences branch 2 times, most recently from 992f49f to d281431 Jan 17, 2020
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 17, 2020

Fixed, rebased, and removed testcase.

OpenRA.Game/Graphics/Util.cs Outdated Show resolved Hide resolved
@pchote pchote force-pushed the pchote:rgba-sequences branch from d281431 to 3eafee8 Jan 17, 2020
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 17, 2020

Fixed.

@abcdefg30 abcdefg30 merged commit 66b8689 into OpenRA:bleed Jan 17, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Jan 17, 2020

@pchote pchote deleted the pchote:rgba-sequences branch Jan 20, 2020
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

4 participants
You can’t perform that action at this time.