feat: Customise grid of NineTileBox #2495

projectitis · 2023-04-14T03:53:06Z

Description

Edit: Closed/re-created MR after rebase with latest changes on main.

The previous implementation of the NineTileBox calculates identically sized tiles in a 3x3 grid and does not allow the user to customise this. For example, a 60x60 pixel sprite will be cut into 20x20 pixel tiles. This MR allows the user to specify the sizes of the fixed-width and fixed-height rows and columns so that a completely custom grid is possible.

Example with the following sprite and custom grid sizes.
Note that the stretchable row and column are only 1 pixel wide/high in this example.

final dialogBubble = NineTileBox.withGrid(
  await Sprite.load(dialogSpriteName),
  leftColumnWidth: 5,
  rightColumnWidth: 4,
  topRowHeight: 4,
  bottomRowHeight: 7,
);

Checklist

I have followed the [Contributor Guide] when preparing my PR.
I have updated/added tests for ALL new/updated/fixed functionality.
I have updated/added relevant documentation in docs and added dartdoc comments with ///.
I have updated/added relevant examples in examples or docs.

Please note the visibility of NineTileBox.center (was private) for testing purposes. Added the @visibleForTesting decorator.

Breaking Change?

Yes, this PR is a breaking change.
No, this PR is not a breaking change.

Related Issues

Closes #2490

Latest from upstream

spydon

Looks good, just a few comments.
Remember that you don't have to create a new PR to update it with new code, you can just continue to push to the same branch and it will automatically update.

examples/lib/stories/rendering/nine_tile_box_custom_grid_example.dart

packages/flame/test/nine_tile_box_test.dart

packages/flame/lib/src/nine_tile_box.dart

projectitis · 2023-04-14T08:43:39Z

Thanks for the review. Happy to make further changes/improvements.

What's the policy with accepting conversations - do you prefer that the reviewer closes their own conversations, or the contributor closes them once they are addressed in a PR?

I decided to close/redo the previous PR because I was already up to 5 commits. I did this because some repo owners really prefer clean PRs - even though the PR can be squashed before it is merged.

spydon · 2023-04-14T08:46:53Z

Thanks for the review. Happy to make further changes/improvements.

Great! I'll ping the others in the team so that they can review too. :)

What's the policy with accepting conversations - do you prefer that the reviewer closes their own conversations, or the contributor closes them once they are addressed in a PR?

Usually the reviewer closes them when they have verified that they are done.

I decided to close/redo the previous PR because I was already up to 5 commits. I did this because some repo owners really prefer clean PRs - even though the PR can be squashed before it is merged.

The amount of commits doesn't matter here because they are squashed into one commit when we merge.

packages/flame/test/nine_tile_box_test.dart

Co-authored-by: Erick <erickzanardoo@gmail.com>

…to feature/ninetilebox-set-grid

spydon · 2023-04-15T14:45:56Z

packages/flame/lib/src/nine_tile_box.dart

+    double leftColumnWidth = 0.0,
+    double rightColumnWidth = 0.0,
+    double topRowHeight = 0.0,
+    double bottomRowHeight = 0.0,


What do you think about dropping Row and Column from the names? Would it be less clear what they mean?

Initially I had just left, right, top, bottom but that was confusing as the values here don't work the same as a regular Rect - but I think _Width, _Height will be ok.

What is your preference?

leftWidth, rightWidth, topHeight, bottomHeight

leftSize, rightSize, topSize, bottomSize

I think the top one is slightly clearer, especially since we usually use size described by a Vector2 that represents both width and height.

would border work? columns and rows made me a little confused here.

So leftBorderWidth etc? Could work, hard do come up with something super clear here.

Personally, border is confusing to me because it suggests it is outside (or surrounds) the NineTileBox. I prefer leftWidth, topHeight etc

Let's go with that then, I think those are fine.
Can you add them to the dartdocs too, explaining what each of them do and it'll be even clearer?
You can refer to arguments in the dartdoc with [leftBorderWidth] etc.

renancaraujo

Made one of our best APIs even better.

renancaraujo · 2023-04-18T14:05:41Z

packages/flame/lib/src/nine_tile_box.dart

+    double leftColumnWidth = 0.0,
+    double rightColumnWidth = 0.0,
+    double topRowHeight = 0.0,
+    double bottomRowHeight = 0.0,


would border work? columns and rows made me a little confused here.

erickzanardo

Overall LGTM, on minor suggestion

erickzanardo · 2023-04-18T23:11:16Z

packages/flame/test/nine_tile_box_test.dart

+      final sprite = Sprite(await loadImage('speech-bubble-1.png'));
+      final nineTileBox = NineTileBox(sprite);
+
+      expect(nineTileBox.tileSize, 30);


Can we use matchers here instead of just putting the raw values

Suggested change

expect(nineTileBox.tileSize, 30);

expect(nineTileBox.tileSize, equals(30));

The same suggestion to the other places in this PR

spydon

Good job!

spydon · 2023-04-19T07:43:23Z

Just needs a formatting fix, you can run melos format and it'll sort it out for you.

projectitis added 2 commits April 14, 2023 15:44

Merge pull request #1 from flame-engine/main

b621b7a

Latest from upstream

Allow user to set manual grid sizes in NineGridBox

7ccc859

spydon reviewed Apr 14, 2023

View reviewed changes

Smaller goldens. Improved tests.

9d7bd99

spydon reviewed Apr 14, 2023

View reviewed changes

packages/flame/lib/src/nine_tile_box.dart Outdated Show resolved Hide resolved

projectitis added 2 commits April 14, 2023 20:51

Formatting

e431ccc

Formatting

b1df566

erickzanardo reviewed Apr 14, 2023

View reviewed changes

packages/flame/test/nine_tile_box_test.dart Outdated Show resolved Hide resolved

projectitis and others added 3 commits April 15, 2023 10:39

Improve tests

fac2388

Co-authored-by: Erick <erickzanardoo@gmail.com>

Grid size inputs to double

aad5b27

Merge remote-tracking branch 'origin/feature/ninetilebox-set-grid' in…

288eb28

…to feature/ninetilebox-set-grid

spydon reviewed Apr 15, 2023

View reviewed changes

renancaraujo approved these changes Apr 18, 2023

View reviewed changes

erickzanardo reviewed Apr 18, 2023

View reviewed changes

Clearer arg names. Improved tests.

5f18567

spydon approved these changes Apr 19, 2023

View reviewed changes

projectitis and others added 2 commits April 19, 2023 19:47

Format (removed space)

b1853f9

Merge branch 'main' into feature/ninetilebox-set-grid

21d7e8b

spydon enabled auto-merge (squash) April 19, 2023 07:56

spydon merged commit a25b0a0 into flame-engine:main Apr 19, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Customise grid of NineTileBox #2495

feat: Customise grid of NineTileBox #2495

projectitis commented Apr 14, 2023

spydon left a comment

projectitis commented Apr 14, 2023

spydon commented Apr 14, 2023

spydon Apr 15, 2023

projectitis Apr 15, 2023 •

edited

spydon Apr 16, 2023

renancaraujo Apr 18, 2023

spydon Apr 18, 2023

projectitis Apr 18, 2023

spydon Apr 18, 2023

renancaraujo left a comment

renancaraujo Apr 18, 2023

erickzanardo left a comment

erickzanardo Apr 18, 2023

spydon left a comment

spydon commented Apr 19, 2023

	expect(nineTileBox.tileSize, 30);
	expect(nineTileBox.tileSize, equals(30));

feat: Customise grid of NineTileBox #2495

feat: Customise grid of NineTileBox #2495

Conversation

projectitis commented Apr 14, 2023

Description

Checklist

Breaking Change?

Related Issues

spydon left a comment

Choose a reason for hiding this comment

projectitis commented Apr 14, 2023

spydon commented Apr 14, 2023

Choose a reason for hiding this comment

projectitis Apr 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renancaraujo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erickzanardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spydon left a comment

Choose a reason for hiding this comment

spydon commented Apr 19, 2023

projectitis Apr 15, 2023 •

edited