Added OnDiagonal Modifier [Modifier 2 of 4]#2693
Open
Crepestrom wants to merge 28 commits intoPixelGuys:masterfrom
Open
Added OnDiagonal Modifier [Modifier 2 of 4]#2693Crepestrom wants to merge 28 commits intoPixelGuys:masterfrom
Crepestrom wants to merge 28 commits intoPixelGuys:masterfrom
Conversation
IntegratedQuantum
requested changes
Mar 14, 2026
| const Encased = struct { | ||
| tag: main.Tag, | ||
| amount: usize, | ||
| range: usize, |
Member
There was a problem hiding this comment.
Please use an optional and if not present check the entire size of the tool. Tools may not be limited to 5×5 in the future.
| } | ||
| for (lowBound..highBound) |dx| { | ||
| const checkedX = x + @as(i32, @intCast(dx - self.range)); | ||
| const checkedY = y - @as(i32, (@intCast(dx - self.range))); |
Member
There was a problem hiding this comment.
Suggested change
| const checkedY = y - @as(i32, (@intCast(dx - self.range))); | |
| const checkedY = y - @as(i32, @intCast(dx - self.range)); |
| for (lowBound..highBound) |dx| { | ||
| const checkedX = x + @as(i32, @intCast(dx - self.range)); | ||
| const checkedY = y - @as(i32, (@intCast(dx - self.range))); | ||
| if (!(dx == 0)) { |
Member
There was a problem hiding this comment.
Suggested change
| if (!(dx == 0)) { | |
| if (dx != 0) { |
| pub const encased = @import("encased.zig"); | ||
| pub const not = @import("not.zig"); | ||
| pub const @"or" = @import("or.zig"); | ||
| pub const OnDiagonal = @import("onDiagonal.zig"); |
Member
There was a problem hiding this comment.
Suggested change
| pub const OnDiagonal = @import("onDiagonal.zig"); | |
| pub const onDiagonal = @import("onDiagonal.zig"); |
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
Member
|
please fix the formatting |
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
Contributor
Author
|
Requested Changes have been made |
IntegratedQuantum
requested changes
Mar 16, 2026
| if (self.range > gridSize) { | ||
| rangeChecked = gridSize; | ||
| } else { | ||
| if (self.range == 0) { |
Member
There was a problem hiding this comment.
Please use an optional instead of magic value 0.
const rangeChecked = @min(self.range orelse gridSize, gridSize);
Contributor
Author
There was a problem hiding this comment.
Ive finally gotten around to it
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
Contributor
|
rip formatting |
IntegratedQuantum
requested changes
Apr 15, 2026
Member
IntegratedQuantum
left a comment
IntegratedQuantum
left a comment
There was a problem hiding this comment.
Please also test this on your own by adding it to a test item and checking if it works. Just by looking at it, I already see a case where it will definitely crash.
| for (lowBound..highBound) |dx| { | ||
| const checkedX = x + @as(i32, @intCast(dx - rangeChecked)); | ||
| const checkedY = y + @as(i32, @intCast(dx - rangeChecked)); | ||
| if ((proceduralItem.getItemAt(checkedX, checkedY) orelse continue).hasTag(self.tag)) count += 1; |
Member
There was a problem hiding this comment.
Missing the dx != 0 check from below
| const highBound = rangeChecked*2 + 1; | ||
| for (lowBound..highBound) |dx| { | ||
| const checkedX = x + @as(i32, @intCast(dx - rangeChecked)); | ||
| const checkedY = y + @as(i32, @intCast(dx - rangeChecked)); |
Member
There was a problem hiding this comment.
This looks wrong, using dx for y. Also dx convention is for delta values, but only dx - rangedCheck is the actual delta.
I'd suggest to rename it.
| const lowBound = 0; | ||
| const highBound = rangeChecked*2 + 1; | ||
| for (lowBound..highBound) |dx| { | ||
| const checkedX = x + @as(i32, @intCast(dx - rangeChecked)); |
Member
There was a problem hiding this comment.
This looks like a crash
Suggested change
| const checkedX = x + @as(i32, @intCast(dx - rangeChecked)); | |
| const checkedX = x + @as(i32, @intCast(dx)) - rangeChecked; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From #2650
-onDiagonal
Default range is 0 (this covers the entire available crafting space on diagonals)