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

Update unit color properties #33

Merged
merged 2 commits into from Sep 12, 2019

Conversation

@ron-murhammer
Copy link
Member

commented Sep 11, 2019

Update unit color properties

Update unit color properties
Update unit color properties

@ron-murhammer ron-murhammer merged commit 531f76e into master Sep 12, 2019

#units.color.ignore provides a comma delimited list of units that aren't impacted by the above units.color properties
#units.color.ignore=factory,fighter
# Unit transform settings (apply effects similar to colorize functions of image editing tools)
#units.transform.color.<player> applies the given color's hue and saturation to the player's units

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Sep 12, 2019

Member

nit: typo with "player's" = player is

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Sep 12, 2019

Member

Same nit on line 22

This comment has been minimized.

Copy link
@ron-murhammer

ron-murhammer Sep 13, 2019

Author Member

Uh not following this? I believe "player's units" is correct?

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Sep 13, 2019

Member

Reviewed my grammar rules, I was wrong - you're originally right @ron-murhammer on the grammar here.

#units.color.flip.Russians=true
#units.color.ignore provides a comma delimited list of units that aren't impacted by the above units.color properties
#units.color.ignore=factory,fighter
# Unit transform settings (apply effects similar to colorize functions of image editing tools)

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Sep 12, 2019

Member

nit: 'transform' and colorize do not really jive, this line seems out of date.

# Unit transform settings (apply effects similar to colorize functions of image editing tools)
#units.transform.color.<player> applies the given color's hue and saturation to the player's units
#units.transform.color.Russians=00BBBB
#units.transform.brightness.<player> allows the brightness (-100 to 100) to be adjusted when above unit color is set

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Sep 12, 2019

Member

Perhaps could be better to replace "unit color" with explicit "units.transform.color". The more explicit mention could help make it more clear and avoid having to scan/reference other parts of the file, and more resilient if props are moved.

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Sep 12, 2019

Member

Second, should there be a mention somewhere that these settings are optional? Maybe something like #units.transform.brightness.<player> (optional) allows the ...

#units.transform.brightness.Russians=25
#units.transform.flip.<player> flips the player's units horizontally
#units.transform.flip.Russians=true
#units.transform.ignore provides a comma delimited list of units that aren't impacted by the above units.transform properties

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Sep 12, 2019

Member

Perhaps would be good to mention that the derived unit images, such as damaged battleship are also included, using that as an example with a comment might help make that more clear.

@DanVanAtta DanVanAtta deleted the ron-murhammer-patch-29 branch Sep 12, 2019

@ron-murhammer

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

Follow up PR: #34

@DanVanAtta For these types of minor changes, its probably easier for you to just make a follow up PR with the improvements and tag me since we don't really review PRs in this repo.

@DanVanAtta

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

@ron-murhammer sounds good for a follow-up flow.

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