Skip to content

fix material colors when less than 5 #2236

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.

Sign up for GitHub

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

Open
SpellDigger wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix material colors when less than 5 #2236

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2a5a718
fix material colors when less than 5
SpellDigger Nov 8, 2025
571f0f0
Attempt 1
SpellDigger Nov 11, 2025
618bf21
final result
SpellDigger Nov 11, 2025
b4b1f53
remove floor from mid
SpellDigger Nov 11, 2025
61ed09a
me forgor
SpellDigger Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/items.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,8 +419,11 @@ const TextureGenerator = struct { // MARK: TextureGenerator
// Calculate the lighting based on the nearest free space:
const lightTL = heightMap[x][y] - heightMap[x + 1][y + 1];
const lightTR = heightMap[x + 1][y] - heightMap[x][y + 1];
var light = 2 - @as(i32, @intFromFloat(@round((lightTL*2 + lightTR)/6)));
light = @max(@min(light, 4), 0);
const mid = @as(f32, @floatFromInt(material.colorPalette.len - 1))/2.0;
const lightScale = mid/2;
const lightVal = (lightTL*2.0 + lightTR)/6.0*lightScale;
Copy link
Member

@IntegratedQuantum IntegratedQuantum Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two magic values here (6.0 should be 3.0, and the /2 in the line above), if you can't find a good name for them, please at least combine them into a single /4.

Copy link
Author

@SpellDigger SpellDigger Nov 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok sherlock how do u want me to combine them then
/3.0 x mid breaks the math in general
/(3.0 x mid) breaks when theres 10 gradients

complaining about magic numbers here is just angers me, because those magic numbers are the exact leftovers u left me to work with and figure out what are they there for

Copy link
Member

@IntegratedQuantum IntegratedQuantum Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because the original code was bad, doesn't give you permission to make things worse.

What I'd suggest is lightScale = mid/4.0 and lightVal = (...)/3.0*... that way we only have one purely magic number instead of two.

Ideally of course you'd get rid of both magic numbers, but I think that would require a larger rewrite of the math, which doesn't seem like a good idea right now.

Copy link
Author

@SpellDigger SpellDigger Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what? this doesnt get rid of any magic numbers unless ur telling me to get rid of the /2 at mid (which is not a magic number cuz its pretty obvious by name im getting the mid point here) but that completly breaks the math

Copy link
Member

@IntegratedQuantum IntegratedQuantum Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The /3.0 is less magic because it's clearly part of an interpolation between two numbers.

var light = @as(i32, @intFromFloat(@round(@floor(mid) - lightVal)));
Copy link
Member

@IntegratedQuantum IntegratedQuantum Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it flooring the mid? To me it seems like this would add a bias when the number of colors is even.

Copy link
Author

@SpellDigger SpellDigger Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without the floor the shading is just messed up

Copy link
Member

@IntegratedQuantum IntegratedQuantum Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image please

light = @max(@min(light, material.colorPalette.len - 1), 0);
img.setRGB(x, 15 - y, material.colorPalette[@intCast(light)]);
} else {
img.setRGB(x, 15 - y, if((x ^ y) & 1 == 0) Color{.r = 255, .g = 0, .b = 255, .a = 255} else Color{.r = 0, .g = 0, .b = 0, .a = 255});
Expand Down