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

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

Closed
eonarheim opened this issue Aug 5, 2024 Discussed in #3166 · 1 comment · Fixed by #3168
Closed

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

eonarheim opened this issue Aug 5, 2024 Discussed in #3166 · 1 comment · Fixed by #3168
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior

Comments

@eonarheim
Copy link
Member

Discussed in #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')

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 ).
  • If we decide to implement this change, we should also add corresponding tests, in accordance with the CONTRIBUTING.md.

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 eonarheim added the bug This issue describes undesirable, incorrect, or unexpected behavior label Aug 5, 2024
@eonarheim
Copy link
Member Author

Fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant