Fix tilemap new weird behaviors #6957
Merged
Conversation
This file contains 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
Collaborator
AlexandreSi
commented
Sep 16, 2024
•
AlexandreSi
commented
- Improve performance display when painting using a PIXI.TilingSprite
- Fix painting when tilemap is rotated
- Allow atlas size to be something else than a tile size multiple and ignore last column and row
- Display error message only when the the tile size is greater than the atlas image
- Do not crash preview if tilemap badly configured
- Add object name in actions and conditions
4ian
reviewed
Sep 16, 2024
Comment on lines
370
to
375
| const allXCoordinates = spritesCoordinatesInTileMapGrid.map(({ x }) => x); | ||
| const allYCoordinates = spritesCoordinatesInTileMapGrid.map(({ y }) => y); | ||
| const minX = Math.min(...allXCoordinates); | ||
| const maxX = Math.max(...allXCoordinates); | ||
| const minY = Math.min(...allYCoordinates); | ||
| const maxY = Math.max(...allYCoordinates); |
There was a problem hiding this comment.
While it looks elegant, probably a better idea to just do a for loop on spritesCoordinatesInTileMapGrid and a if (x < minX) minX = x;, because otherwise you're allocating 2 arrays of easily 500-200 elements at each render (not bad, but ok) and calling Math.min/max with 500-2000 parameters - JS engines are certainly not happy with this.
A good old for loop while be easily optimized by JS engines and even CPUs.
There was a problem hiding this comment.
Yeah good idea, it was a bit brainless 😅
4ian
reviewed
Sep 16, 2024
Extensions/TileMap/JsExtension.js
Outdated
| @@ -940,7 +955,7 @@ const defineSimpleTileMap = function (extension, _, gd) { | |||
| 'RemoveTileAtGridCoordinates', | |||
| _('Remove tile (on the grid)'), | |||
| _('Remove the tile at the grid coordinates.'), | |||
| _('Remove tile at grid coordinates _PARAM1_ ; _PARAM2_'), | |||
| _('Remove tile at grid coordinates _PARAM1_ ; _PARAM2_ of _PARAM0_'), | |||
There was a problem hiding this comment.
Suggested change
| _('Remove tile at grid coordinates _PARAM1_ ; _PARAM2_ of _PARAM0_'), | |
| _('Remove tile of _PARAM0_ at grid coordinates _PARAM1_;_PARAM2_'), |
4ian
reviewed
Sep 16, 2024
Extensions/TileMap/JsExtension.js
Outdated
| @@ -922,7 +937,7 @@ const defineSimpleTileMap = function (extension, _, gd) { | |||
| _('Flip tile horizontally (on the grid)'), | |||
| _('Flip tile horizontally at grid coordinates.'), | |||
| _( | |||
| 'Flip tile horizontally at grid coordinates _PARAM1_ ; _PARAM2_: _PARAM3_' | |||
| 'Flip tile horizontally at grid coordinates _PARAM1_ ; _PARAM2_: _PARAM3_ of _PARAM0_' | |||
There was a problem hiding this comment.
Suggested change
| 'Flip tile horizontally at grid coordinates _PARAM1_ ; _PARAM2_: _PARAM3_ of _PARAM0_' | |
| 'Flip tile horizontally at grid coordinates _PARAM1_;_PARAM2_ of _PARAM0_: _PARAM3_' |
4ian
reviewed
Sep 16, 2024
Extensions/TileMap/JsExtension.js
Outdated
| @@ -903,7 +918,7 @@ const defineSimpleTileMap = function (extension, _, gd) { | |||
| _('Flip tile vertically (on the grid)'), | |||
| _('Flip tile vertically at grid coordinates.'), | |||
| _( | |||
| 'Flip tile vertically at grid coordinates _PARAM1_ ; _PARAM2_: _PARAM3_' | |||
| 'Flip tile vertically at grid coordinates _PARAM1_ ; _PARAM2_: _PARAM3_ of _PARAM0_' | |||
There was a problem hiding this comment.
Suggested change
| 'Flip tile vertically at grid coordinates _PARAM1_ ; _PARAM2_: _PARAM3_ of _PARAM0_' | |
| 'Flip tile vertically at grid coordinates _PARAM1_;_PARAM2_ of _PARAM0_: _PARAM3_' |
There was a problem hiding this comment.
I prefer spaces around the semicolon. Otherwise, expressions are hard to read:
compared to:
4ian
reviewed
Sep 16, 2024
Extensions/TileMap/JsExtension.js
Outdated
| @@ -832,7 +847,7 @@ const defineSimpleTileMap = function (extension, _, gd) { | |||
| _('Flip tile vertically (at position)'), | |||
| _('Flip tile vertically at scene coordinates.'), | |||
| _( | |||
| 'Flip tile vertically at scene coordinates _PARAM1_ ; _PARAM2_: _PARAM3_' | |||
| 'Flip tile vertically at scene coordinates _PARAM1_ ; _PARAM2_: _PARAM3_ of _PARAM0_' | |||
There was a problem hiding this comment.
Everywhere: don't put "of " after a boolean but after the scene coordinates.
For coordinates, use X;Y, not X ; Y
4ian
reviewed
Sep 16, 2024
Extensions/TileMap/JsExtension.js
Outdated
| @@ -815,7 +830,7 @@ const defineSimpleTileMap = function (extension, _, gd) { | |||
| 'TileIdAtPosition', | |||
| _('Tile (at position)'), | |||
| _('the id of the tile at the scene coordinates'), | |||
| _('the tile id at scene coordinates _PARAM3_ ; _PARAM4_'), | |||
| _('the tile id at scene coordinates _PARAM3_ ; _PARAM4_ of _PARAM0_'), | |||
There was a problem hiding this comment.
Suggested change
| _('the tile id at scene coordinates _PARAM3_ ; _PARAM4_ of _PARAM0_'), | |
| _('the tile id of _PARAM0_ at scene coordinates _PARAM3_ ; _PARAM4_'), |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.