excaliburjs / Excalibur

🎮 Your friendly TypeScript 2D game engine for the web 🗡️
https://excaliburjs.com
BSD 2-Clause "Simplified" License
1.77k stars 189 forks source link

Make return values nullable for Gif.toSprite and TileMap.getTileByIndex methods #3167

Closed eonarheim closed 1 month ago

eonarheim commented 1 month ago

Discussed in https://github.com/excaliburjs/Excalibur/discussions/3166

Originally posted by **ambmcmdmem626** August 5, 2024 **Version: 0.29.3** ## Premise If an index outside the array is specified for the following two methods, either `undefined` is returned (`TileMap.getTileByIndex`) or an exception is thrown (`Gif.toSprite`). \* Content of the Exception: `TypeError: Cannot read properties of undefined (reading 'toSprite')` - https://github.com/excaliburjs/Excalibur/blob/2e4341f783b0329a960d6f3b2edbafa4d66d0221/src/engine/Resources/Gif.ts#L79-L86 - https://github.com/excaliburjs/Excalibur/blob/2e4341f783b0329a960d6f3b2edbafa4d66d0221/src/engine/TileMap/TileMap.ts#L459-L464 ## Main topic Is there a reason why the return values of these methods are not nullable? I believe the issue can be addressed by modifying the code as shown below: - For the comments added to the JSDoc, I referred to `TileMap.getTileByPoint()`( [link](https://github.com/excaliburjs/Excalibur/blob/2e4341f783b0329a960d6f3b2edbafa4d66d0221/src/engine/TileMap/TileMap.ts#L479) ). - If we decide to implement this change, we should also add corresponding tests, in accordance with the [CONTRIBUTING.md](https://github.com/excaliburjs/Excalibur/blob/main/.github/CONTRIBUTING.md#tests). ## Suggested Code Changes ``` // src/engine/Resources/Gif.ts /** * Return a frame of the gif as a sprite by id * @param id * returns `null` if no Sprite was found. */ public toSprite(id: number = 0): Sprite | null { const sprite = this._textures[id]?.toSprite() ?? null; return sprite; } ``` ``` // src/engine/TileMap/TileMap.ts /** * Returns the {@apilink Tile} by index (row major order) * returns `null` if no Tile was found. */ public getTileByIndex(index: number): Tile | null { return this.tiles[index] ?? null; } ``` Thank you so much for reading this!
eonarheim commented 1 month ago

Fixed!