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

Removed PlayerPaletteFromCurrentTileset #13944

Merged
merged 1 commit into from Sep 17, 2017

Conversation

Projects
None yet
5 participants
@Mailaender
Member

Mailaender commented Aug 30, 2017

Use PlayerPaletteFromCurrentTileset for Tiberian Sun rather then two palettes with tileset filters.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 31, 2017

Contributor

I haven't had time to look more closely, but according to Travis/lint this breaks a lot of palette references.

Contributor

reaperrr commented Aug 31, 2017

I haven't had time to look more closely, but according to Travis/lint this breaks a lot of palette references.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Aug 31, 2017

Member

This is strange and unexpected as it works in-game.

Member

Mailaender commented Aug 31, 2017

This is strange and unexpected as it works in-game.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 31, 2017

Member

This doesn't look like a normal player palette, rather a normal palette called "player". IIRC it is used for things that should be drawn in unittem/unitsno but not be remapped with the owner's colour.

Member

pchote commented Aug 31, 2017

This doesn't look like a normal player palette, rather a normal palette called "player". IIRC it is used for things that should be drawn in unittem/unitsno but not be remapped with the owner's colour.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Aug 31, 2017

Member

It is used as a base for the PlayerColorPalette which is remapped per player. I am not sure if I stumbled upon a lint bug here. The PaletteReference tags seem alright.

Member

Mailaender commented Aug 31, 2017

It is used as a base for the PlayerColorPalette which is remapped per player. I am not sure if I stumbled upon a lint bug here. The PaletteReference tags seem alright.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 31, 2017

Contributor

PlayerPaletteFromCurrentTileset sets the PaletteDefinition attribute to IsPlayerPalette = true, which sort of makes sense, given its name. This is what causes the lint errors.

In theory, since it's only a normal palette at this point and PlayerColorPalette does the same, I guess you could remove the (true) from PlayerPaletteFromCurrentTileset's attribute, since the latter palette alone doesn't really do anything player-palette-specific apart from its name, so I'm not sure it should have that attribute set to true to begin with. You'd have to make it implement IProvidesAssetBrowserPalettes (like PaletteFromFile) to keep the palette(s) available in the asset browser then.

I suggest waiting a bit for other feedback, though, as I might have overlooked something or others might disagree with that change.

Contributor

reaperrr commented Aug 31, 2017

PlayerPaletteFromCurrentTileset sets the PaletteDefinition attribute to IsPlayerPalette = true, which sort of makes sense, given its name. This is what causes the lint errors.

In theory, since it's only a normal palette at this point and PlayerColorPalette does the same, I guess you could remove the (true) from PlayerPaletteFromCurrentTileset's attribute, since the latter palette alone doesn't really do anything player-palette-specific apart from its name, so I'm not sure it should have that attribute set to true to begin with. You'd have to make it implement IProvidesAssetBrowserPalettes (like PaletteFromFile) to keep the palette(s) available in the asset browser then.

I suggest waiting a bit for other feedback, though, as I might have overlooked something or others might disagree with that change.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 31, 2017

Member

The trait should be renamed if it isn't really creating player palettes.

Member

pchote commented Aug 31, 2017

The trait should be renamed if it isn't really creating player palettes.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 31, 2017

Contributor

The trait should be renamed if it isn't really creating player palettes.

We should rename the PlayerPalette field in the tileset definition as well then. Question is, to what.
ActorPalette?

Contributor

reaperrr commented Aug 31, 2017

The trait should be renamed if it isn't really creating player palettes.

We should rename the PlayerPalette field in the tileset definition as well then. Question is, to what.
ActorPalette?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 31, 2017

Member

Hardcoding trait-specific data in the tilesets is awful to begin with. It goes directly against our long term goal of removing hardcoded mod-specific stuff from the engine. Thinking about it, the cleanest solution would be to remove PlayerPaletteFromCurrentTileset completely and then use PaletteFromFile with tileset filters everywhere.

Member

pchote commented Aug 31, 2017

Hardcoding trait-specific data in the tilesets is awful to begin with. It goes directly against our long term goal of removing hardcoded mod-specific stuff from the engine. Thinking about it, the cleanest solution would be to remove PlayerPaletteFromCurrentTileset completely and then use PaletteFromFile with tileset filters everywhere.

@Mailaender Mailaender changed the title from Use PlayerPaletteFromCurrentTileset for Tiberian Sun to Removed PlayerPaletteFromCurrentTileset Aug 31, 2017

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Aug 31, 2017

Member

Went with a trait removal then.

Member

Mailaender commented Aug 31, 2017

Went with a trait removal then.

@@ -185,7 +185,6 @@ public class TileSet
public readonly string Id;
public readonly int SheetSize = 512;
public readonly string Palette;

This comment has been minimized.

@pchote

pchote Aug 31, 2017

Member

The same applies here too.

@pchote

pchote Aug 31, 2017

Member

The same applies here too.

This comment has been minimized.

@Mailaender

Mailaender Sep 1, 2017

Member

https://github.com/Mailaender/OpenRA/tree/move-palette-from-tileset will be a lot more invasive. I probably won't finish it.

@Mailaender

Mailaender Sep 1, 2017

Member

https://github.com/Mailaender/OpenRA/tree/move-palette-from-tileset will be a lot more invasive. I probably won't finish it.

This comment has been minimized.

@pchote

pchote Sep 16, 2017

Member

Ok, lets focus on just PlayerPalette here then.

@pchote

pchote Sep 16, 2017

Member

Ok, lets focus on just PlayerPalette here then.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 1, 2017

Contributor

Needs rebase.

Contributor

reaperrr commented Sep 1, 2017

Needs rebase.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Sep 1, 2017

Member

Rebased.

Member

Mailaender commented Sep 1, 2017

Rebased.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Sep 14, 2017

Member

Needs another rebase... :/

Member

abcdefg30 commented Sep 14, 2017

Needs another rebase... :/

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Sep 16, 2017

Member

Rebased and added StringComparison.Ordinal which my new MonoDevelop 7 suggested.

Member

Mailaender commented Sep 16, 2017

Rebased and added StringComparison.Ordinal which my new MonoDevelop 7 suggested.

@pchote

pchote approved these changes Sep 16, 2017

@pchote pchote added the PR: Needs +2 label Sep 16, 2017

@reaperrr reaperrr merged commit 3af0b1a into OpenRA:bleed Sep 17, 2017

2 checks passed

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

@Mailaender Mailaender deleted the Mailaender:playercolorpalette-fromtileset branch Sep 17, 2017

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