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

Robots and entities should take on underlying cell background unless overridden #1662

Open
kostmo opened this issue Nov 26, 2023 · 1 comment · May be fixed by #1672
Open

Robots and entities should take on underlying cell background unless overridden #1662

kostmo opened this issue Nov 26, 2023 · 1 comment · May be fixed by #1672
Labels
C-Moderate Effort Should take a moderate amount of time to address. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. T-UI Involves the user interface. Z-Feature A new feature to be added to the game.

Comments

@kostmo
Copy link
Member

kostmo commented Nov 26, 2023

Currently, any entity or robot with an unspecified background attribute gets a black background by default.

A potential advantage to the current behavior, at least for robots, is better visibility contrast. However, it arguably "breaks immersion" and is less aesthetically pleasing than taking on the underlying background. And anyway it shall still be possible to explicitly set the robot's background color as a custom attr, if desired in some scenario.

@kostmo kostmo added Z-Feature A new feature to be added to the game. T-UI Involves the user interface. labels Nov 26, 2023
@byorgey
Copy link
Member

byorgey commented Dec 5, 2023

In principle I agree with this idea, although it may require changing some color schemes. There are probably some current entity/terrain color combinations that would be difficult to see.

mergify bot pushed a commit that referenced this issue Dec 12, 2023
Towards #1662

Prerequisite for #1672

To allow a robot-occupied cell to take on the underlying terrain color as the background color of its cell, the terrain color must be representable strictly as a "background".  However, currently, the `dirt`, `stone`, and `grass` terrains are represented by a half-shaded **foreground** glyph upon `black` background.

Currently the ["Medium Shade" unicode character](https://www.compart.com/en/unicode/U+2592) (`▒`) is used  to "blend" a somewhat bright color with a black background, resulting in a moderately dark color in the terminal for `dirt`, `stone`, and `grass`.  However, these same dark colors are not reproducible in the 240-color scheme without this foreground+background blending trick; the closest approximations as a background-only or foreground-only color come out quite a bit lighter.

## Visual comparison

Using:

    scripts/play.sh -i creative --seed 2 --autoplay

| Before | After |
| --- | --- |
| ![Screenshot from 2023-12-03 23-33-15](https://github.com/swarm-game/swarm/assets/261693/edeeaeac-13e0-4641-9822-773fdb20f1d4) | ![Screenshot from 2023-12-03 23-32-36](https://github.com/swarm-game/swarm/assets/261693/ae5a5b5d-aa69-4580-b7e1-85eec21b4aeb) |

## Possible approaches

So, we need to decide whether to:
1. Accept the new lighter colors
2. Choose new, alternative terrain colors that may be darker given the 240-color palette
3. Abandon support for 240 colors and assume "full" color terminals
4. Abandon efforts to passthrough terrain color as background of robot cells
@kostmo kostmo added the C-Moderate Effort Should take a moderate amount of time to address. label Feb 19, 2024
@byorgey byorgey added the S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Moderate Effort Should take a moderate amount of time to address. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. T-UI Involves the user interface. Z-Feature A new feature to be added to the game.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants