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

Fix putSliceWithBorders extention function for Bitmap class #2144

Merged
merged 2 commits into from Feb 13, 2024

Conversation

jobe-m
Copy link
Member

@jobe-m jobe-m commented Feb 6, 2024

The function putSliceWithBorders is used to create an extruded tileset in LdtkView. The tiles were not extruded correctly into top and bottom left corners. This commit fixes that. It also adds a unit test which reproduces the error.

Incorrectly expanded example of a rectangle:

image

The function putSliceWithBorders is used to create an extruded tileset
in LdtkView. The tiles were not extruded correctly into top and bottom
left corners. This commit fixes that. It also adds a unit test which
reproduces the error.
@jobe-m jobe-m requested a review from soywiz February 6, 2024 16:50
@@ -53,8 +53,8 @@ fun Bitmap.putSliceWithBorder(x: Int, y: Int, bmp: BmpSlice, border: Int = 1) {
// Vertical replicate
for (n in 1..border) {
val rwidth = width + border * 2
this.copy(x, y, this, x, y - n, rwidth, 1)
this.copy(x, y + height - 1, this, x, y + height - 1 + n, rwidth, 1)
this.copy(x - border, y, this, x - border, y - n, rwidth, 1)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be rwidth + border * 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you exatly mean should I put that term?
I did the fix in a rush. Let me check it carefully with a bit more time on Friday :)
I will create also a "better" (more colorful) tile for visually checking if the expansion of the border works as we expect.

Copy link
Member

Choose a reason for hiding this comment

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

fun Bitmap.copy(srcX: Int, srcY: Int, dst: Bitmap, dstX: Int, dstY: Int, width: Int, height: Int)

I mean, if border was not taken into account and if position is displaced now, width might need to be extended?

For example, let's imagine a segment: x=10, width=10 (right = 20)
If I want to add one pixel on both sides to the line: x=9, width=12 (right=21)

So subtracting N to the x (left), requires adding N*2 to the width, or N to the right coordinate.
In the case we are extending the same amount of pixels on both sides of the segment.

Not sure if that was the case here, but asked just in case.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I checked now after my Friday stream again and my implementation actually adds the border twice. So, if I add border = 2 than 4 pixels are added to each side of the tile :)

I need to check again...

Copy link
Member Author

@jobe-m jobe-m Feb 12, 2024

Choose a reason for hiding this comment

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

I don't see the border error from last Friday anymore.

I checked again now and I could not find a problem anymore with my initial changes. Here is a screenshot for border = 1:

image

So it expands correctly by one pixel in each direction. I checked that visually with border = 2, 3 and 5 and it was correctly expanding with 2 resp. 3 and 5 pixels.

@jobe-m jobe-m marked this pull request as draft February 7, 2024 08:15
@@ -27,6 +27,25 @@ class BitmapSliceTest {
assertEquals("Rectangle(x=24, y=24, width=24, height=24)", bmp.sliceWithSize(16, 16, 32, 32).sliceWithSize(8, 8, 40, 40).bounds.toString())
}

@Test
fun testPutSliceWithBorder() {
Copy link
Member

@soywiz soywiz Feb 7, 2024

Choose a reason for hiding this comment

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

Regarding test. Maybe the Bitmap32 can be converted into a Bitmap1/Bitmap8 and then:

assertEquals(
"""
X...
.X..
.X..
...X
""".trimIndent(),
bmp.toLines(".X").joinToString("\n")

fun Bitmap.tryToExactBitmap8(): Bitmap8? {

bmp.tryToExactBitmap8().toLines(".*#")!!.joinToString("\n")

or use a reference image. Since we are in korge-core, we should be able to generate screenshots.

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked using Bitmap1 in the unit test but putWithBorder() is not implemented for Bitmap1. What seems to be missing is the implementation of slice() for Bitmap1.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since Slice() is not implemented for Bitmap1 I would like to keep the unit test as it is now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can try with Bitmap8 indeed and create an imput and output image with indexes...

@jobe-m jobe-m marked this pull request as ready for review February 12, 2024 15:48
@jobe-m
Copy link
Member Author

jobe-m commented Feb 12, 2024

Now unit test uses Bitmap8 and has a better visual test

Copy link
Member

@soywiz soywiz left a comment

Choose a reason for hiding this comment

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

LGTM

@soywiz soywiz enabled auto-merge (squash) February 13, 2024 11:00
@soywiz soywiz merged commit df41c99 into main Feb 13, 2024
9 checks passed
@soywiz soywiz deleted the fix-put-slice-with-borders branch February 13, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants