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

Add xPosToTile and yPosToTile #863

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bruh-9000
Copy link
Contributor

Rationale for implementing this:

Helpful for preventing repetition for converting positions to a tile, whether for trying to get the tile ID of a position, or centering a unit on their tile, etc

Referenced Issue:

Github issue of the requested feature

Demo game JSON:

Haven't done yet, will update when I do

@bruh-9000 bruh-9000 marked this pull request as draft March 6, 2024 17:23
@minotalen
Copy link
Contributor

I like it, but why not do this with instead of doing it for x and y individually? Would def result in cleaner & easier tile edit scripts.

@bruh-9000
Copy link
Contributor Author

Well it's cuz tile width and tile height having different values

@bruh-9000 bruh-9000 marked this pull request as ready for review March 11, 2024 19:03
@minotalen
Copy link
Contributor

var tileWidth = 64 || taro.game.data.map.tilewidth;
this always takes the 64 by default. I would much rather have access to taro.game.data.map.tilewidth and taro.game.data.map.tileheight instead. We currently have width and height enforced to be the same, but we future-proof this by allowing both.

@minotalen minotalen closed this Mar 14, 2024
@bruh-9000
Copy link
Contributor Author

Why'd you close? Can't I just fix the 64 being default

@nickyvanurk nickyvanurk reopened this Mar 15, 2024
@minotalen
Copy link
Contributor

Why'd you close? Can't I just fix the 64 being default

Sorry, there's been a misunderstanding then. After some discussion we decided that we don't want this to be an extra action since the operation is pretty basic ( floor of xPos divided by tileWidth) and can be easily scripted. Having an action for it saves time in one case while increasing bloat for all other cases.
We can however create variables that return taro.game.data.map.tilewidth and taro.game.data.map.tileheight.

@bruh-9000
Copy link
Contributor Author

While it is quite basic, it was very commonly used for things, such as aligning an entity to a tile, detecting what tile type an entity is on, making UI features such as a minimap, etc. This can be quite useful. I suggest asking the community if they think it would be a useful addition, such as in https://discord.com/channels/303543835345158145/1099077055066415134

Copy link
Contributor

@PineappleBrain PineappleBrain left a comment

Choose a reason for hiding this comment

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

Forget to submit this few weeks ago

@@ -1079,6 +1079,22 @@ var ParameterComponent = TaroEntity.extend({

break;

case 'xPosToTile':
var tileWidth = 64 || taro.game.data.map.tilewidth;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this line work as expect? It will never use taro.game.data.map.tileWidth / taro.game.data.map.tileHeight logically
image

Also taro.game.data.map.tilewidth refers to the tileWidth of the map only, it wont change even creator select force 64x tile size
Use taro.scaleMapDetails.tileWidth

if (taro.game.data.defaultData.dontResize) {
taro.scaleMapDetails = {
scaleFactor: { x: 1, y: 1 },
shouldScaleTilesheet: false,
tileWidth: gameMap.tilewidth,
tileHeight: gameMap.tileheight,
originalTileHeight: gameMap.tileheight,
originalTileWidth: gameMap.tilewidth
};
} else {
gameMap.originalTileWidth = gameMap.tilewidth;
gameMap.originalTileHeight = gameMap.tileheight;
taro.scaleMapDetails = {
scaleFactor: {
x: 64 / gameMap.originalTileWidth,
y: 64 / gameMap.originalTileHeight
},
originalTileHeight: gameMap.originalTileHeight,
originalTileWidth: gameMap.originalTileWidth,
tileWidth: 64,
tileHeight: 64,
shouldScaleTilesheet: false

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

4 participants