Skip to content

fix: account for tilePos.x getting out of the level boundary#1043

Open
benhuangbmj wants to merge 4 commits into
kaplayjs:masterfrom
benhuangbmj:master
Open

fix: account for tilePos.x getting out of the level boundary#1043
benhuangbmj wants to merge 4 commits into
kaplayjs:masterfrom
benhuangbmj:master

Conversation

@benhuangbmj

@benhuangbmj benhuangbmj commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Please describe what issue(s) this PR fixes.

levelComp.getAt returns incorrect results when tilePos.x is beyond the scope of the level.

Summary

When the tilePos is beyond the scope of the level, the getAt method should return an empty array. The tile2Hash function already accounts for tilePos.y getting outside of the level. However, it doesn't work for tilePos.x properly. I added a simple check in the getAt method without touching any other part of the code to avoid unexpected breaking behavior.

  • Changeloged

@dragoncoder047

Copy link
Copy Markdown
Contributor

The tile2hash function already accounts for tilePos.y getting outside of the level.

you sure about that?

https://github.com/benhuangbmj/kaplay/blob/7dfe1a634d0d821459ffcf136a6f48699a57d5c7/src/ecs/components/level/level.ts#L141

@benhuangbmj

benhuangbmj commented Feb 19, 2026

Copy link
Copy Markdown
Contributor Author

The tile2hash function already accounts for tilePos.y getting outside of the level.

you sure about that?

https://github.com/benhuangbmj/kaplay/blob/7dfe1a634d0d821459ffcf136a6f48699a57d5c7/src/ecs/components/level/level.ts#L141

What I meant was, suppose the level has m rows and n columns, then its tiles are indexed from 0 to m*n - 1. As long as 0 <= tilePos.x < m*n, then whenever tilePos.y >= m or tilePos.y < 0, tile2Hash will return a number that is beyond the proper index range, henceforth getAt will just return an empty array.

However, without the "0 <= tilePos.x < m*n" check, tile2Hash may potentially return an index within the proper index range even if tilePos itself is beyond the level boundary. And that's why I proposed the current fix.

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.

2 participants