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

Add support for GIMP and JASC palettes (with per-color alpha) #14851

Closed
wants to merge 3 commits into
base: bleed
from

Conversation

Projects
None yet
6 participants
@reaperrr
Copy link
Contributor

reaperrr commented Feb 24, 2018

Supersedes #13913.

Compared to #13913, following changes and additions were made:

  • merged GIMP/JASC support into a single trait
  • replaced the limited-use Tileset with Tilesets + ExcludeTilesets
  • Added support for per-color alpha
  • updated testcase commits to account for per-color alpha (RA effects, TS units) and make testing quicker
@@ -23,6 +23,10 @@
Filename: temperat.pal
ShadowIndex: 4
AllowModifiers: false
PaletteFromGimpFile:

This comment has been minimized.

@Phrohdoh

Phrohdoh Feb 24, 2018

Member

This needs to be PaletteFromGimpOrJascFile, right?

This comment has been minimized.

@reaperrr

reaperrr Feb 24, 2018

Author Contributor

Stupid me, of course.

@Phrohdoh

This comment has been minimized.

Copy link
Member

Phrohdoh commented Feb 24, 2018

There are quite a few PaletteFromGimpFiles in yaml that need to be PaletteFromGimpOrJascFile.

@reaperrr reaperrr force-pushed the reaperrr:more-pals-v2 branch from 8336f67 to d8a2ed9 Feb 24, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Feb 24, 2018

Updated.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

Cooooool. 👍

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 24, 2018

Do you want to merge the example commit, or is that just a testcase? (IMO they belong in https://github.com/OpenRA/ArtSrc, not here)

public readonly string Filename = null;

[Desc("Map listed indices to shadow. Ignores previous color.")]
public readonly int[] ShadowIndex = { };

This comment has been minimized.

@pchote

pchote Feb 24, 2018

Member

Modders can define shadows (including alpha) directly in the palette, so this is redundant.


if (r == 0 && g == 0 && b == 0)
colors[i] = 0;
else if (a == 0)

This comment has been minimized.

@pchote

pchote Feb 24, 2018

Member

Doesn't this prevent fully transparent colours?

This comment has been minimized.

@pchote

pchote Feb 24, 2018

Member

The issues here could be solved by treating r == 0 && g == 0 && b == 0 && a == 0 (transparent) as distinct from r == 0 && g == 0 && b == 0 (black)

This comment has been minimized.

@reaperrr

reaperrr Feb 24, 2018

Author Contributor

But alpha of 0 means zero transparency...?

This comment has been minimized.

@pchote

pchote Feb 24, 2018

Member

Sorry yes, I meant non-transparent black on the case above.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Feb 24, 2018

Do you want to merge the example commit?

No, I'll rename the commit titles to make that more clear.

@reaperrr reaperrr force-pushed the reaperrr:more-pals-v2 branch 2 times, most recently from 17b894c to 1962bd0 Feb 25, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Feb 25, 2018

Updated.

Palette index 0 is now always converted to fully transparent regardless of original color, in favor of otherwise keeping the code as simple and index-hack-free as possible.
Removed ShadowIndex in favor of using a black or dark-grey color + alpha.

Also updated testcases:

  • GIMP testcase is RA, with explosion effects showcasing per-color alpha (transparency increases the darker the explosion color gets).
  • JASC testcase is TS, showcasing non-black index 0 being converted to transparent, black + 128 alpha serving as shadow (color index 1).

@reaperrr reaperrr force-pushed the reaperrr:more-pals-v2 branch from 1962bd0 to 7922035 Mar 4, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 4, 2018

Rebased to fix the AppVeyor issue.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 8, 2018

@GraionDilach is your +1 still valid after the updates?

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 8, 2018

I'm not happy with the slot 0 aspect, Photoshop throws the unused color slots to the end of it's palette when saving an indexed image, which might throw people off if they do a direct split to SHP/palette combo.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 8, 2018

On a second thought, I'm fine with an explicit mention of the slot 0 hardcode in the trait description.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 8, 2018

OK. We can always update the trait if it turns out to be an issue.

@reaperrr reaperrr force-pushed the reaperrr:more-pals-v2 branch from 7922035 to 4388a9d Mar 8, 2018

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@Mailaender Mailaender closed this Mar 8, 2018

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Mar 8, 2018

Cherry-picked to bleed without the test cases 1205450.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 8, 2018

@Mailaender Don't forget the changelog entry.

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Mar 8, 2018

@reaperrr reaperrr deleted the reaperrr:more-pals-v2 branch Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.