-
-
Notifications
You must be signed in to change notification settings - Fork 922
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
fix: Size for SpriteComponent.fromImage
should be nullable
#3054
Conversation
@@ -72,7 +71,6 @@ class SpriteComponent extends PositionComponent | |||
srcPosition: srcPosition, | |||
srcSize: srcSize, | |||
), | |||
autoResize: autoResize, | |||
paint: paint, | |||
position: position, | |||
size: size ?? srcSize ?? image.size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SpriteComponent.fromImage
should not explicitly set size as srcSize
or image.size
. So something like this should be enough.
size: size ?? srcSize ?? image.size, | |
size: size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for that? Then it'll have a different behavior than the other constructor right? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. SpriteComponent.fromImage
internally invokes the same constructor.
}) : this( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that makes a lot of sense then!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... should I commit changes based on @ufrshubham's suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, please do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, commit that change and then autoResize
can be left as it was earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fa-fifi can you update the title and description of the PR to be inline with what it actually does. Also, some tests for SpriteComponent.fromImage
would be good.
SpriteComponent.fromImage
should be nullable
I have updated the title and the description for this PR. Feel free to edit them if you think they are not suitable since English is not my mother tongue. There are some other stuff that I have to focus on rn, so I'll revisit this PR again in the next couple days to update the test cases for |
Description
I received an assertion error after I set
autoResize
to true when I am usingSpriteComponent.fromImage
component. It's kinda weird for me that the message tells me "If size is set, autoResize should be false or size should be null when autoResize is true.'', but the thing is I never set the size for that image yet. Then, I check the code and I found out that the size will never be null anyway, so theautoresize
should always be false.In this PR, I just remove all the fallback value for
size
, so it would not give an assertion error whensize
is not being set yet while theautoresize
is true.Checklist
docs
and added dartdoc comments with///
.examples
ordocs
.Breaking Change?