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

bug: Add missing type definition in SpriteConfig #6770

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

Conversation

mischulz14
Copy link

Please do not update the README or Change Log, we will do this when we merge your PR.

This PR (delete as applicable)

  • Fixes a bug

Describe the changes below:

This PR adds the missing type definition for the boolean useSpriteSheet property in the SpriteConfig.js file, that is later used conditionally in a function in the CreateFromTiles.js file.

Closes #6752

@rexrainbow
Copy link
Contributor

Parameter useSpriteSheet is for CreateFromTiles, not for creating sprite.
Add a new file for CreateFromTiles dedicated instead of putting useSpriteSheet into SpriteConfig would be better, imo.

@mischulz14
Copy link
Author

Parameter useSpriteSheet is for CreateFromTiles, not for creating sprite.
Add a new file for CreateFromTiles dedicated instead of putting useSpriteSheet into SpriteConfig would be better, imo.

Ok, thanks for the insight, I'll refactor my code! (:

@mischulz14
Copy link
Author

@rexrainbow Upon reviewing the code again, I don't think it would make sense to create a dedictated file.

This is because the config object that gets checked here:

// line 83
 if (config.hasOwnProperty('useSpriteSheet')) // this config is a deep copy of the spriteConfig passed to the function
            {
                config.key = tile.tileset.image;
                config.frame = tile.index - 1;
            }

is the spritesheet config created in the SpriteConfig.js file. That leads me to believe the useSpriteSheet property has to come from the spriteConfig declared in the upper scope of the file.

// line 22
* @param {Phaser.Types.GameObjects.Sprite.SpriteConfig} spriteConfig - The config object to pass into the Sprite creator (i.e. scene.make.sprite).

HOWEVER, if you meant to introduce a completely new parameter to the function called useSpriteSheet, that would change things. Any feedback is appreciated (:

@rexrainbow
Copy link
Contributor

When user uses config(SpriteConfig) for creating sprite game object, then user read the SpriteConfig source file, will user know the purpose or usage of useSpriteSheet parameter if user does not survey it deeply?
User might find that it is for tileset, and it can be ignored for creating sprite game object at last. It will introduce more effort to understand the usage of SpriteConfig, imo.

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.

Missing type define of useSpriteSheet parameter
2 participants