fix null reference and shows entire frame bug. #20
Conversation
ghost
mentioned this pull request
js/rpg_core/Sprite.js
Outdated
| Sprite.prototype._isInBitmapRect = function(x, y, w, h) { | ||
| return (this._bitmap && x + w > 0 && y + h > 0 && | ||
| x < this._bitmap.width && y < this._bitmap.height); | ||
| x < this._bitmap.width && y < this._bitmap.height); |
There was a problem hiding this comment.
Found format-only changes! :-)
js/rpg_core/Sprite.js
Outdated
| if (this._frame.width === 0 && this._frame.height === 0) { | ||
| this._frame.width = this._bitmap.width; | ||
| this._frame.height = this._bitmap.height; | ||
| if(this._requestBitmap === this._bitmap){ |
There was a problem hiding this comment.
I don't know why this._requestBitmap is defined, because it's always the same as this._bitmap.
There was a problem hiding this comment.
nope. _onBitmapLoad is async.
There is some chance to change Bitmap while loading is not completed.
There was a problem hiding this comment.
Of course it's async, so bitmap may be changed.
But... this._requestBitmap is changed at the same time. (69-70)
So, this condition-expression will always be true.
There was a problem hiding this comment.
Oops you are right.
Something went wrong, I'll re-test it.
|
I found a description similar to |
|
TilingSprite is already guarded by line: 183 |
|
OH, really? |
|
Sprite needs clearing frame, This can cause showing entire frame. |
|
Hmm, I still don't understand your intentions. In this code, frame is refreshed when bitmap changes null to non-null. |
|
I deferred only clearing entire frame. |
| } else { | ||
| if(!this._bitmap){ | ||
| this._refreshFrame = true; | ||
| }else if(this._bitmap && value){ |
There was a problem hiding this comment.
this condition-expression can be written shorter.
Written long for readability?
There was a problem hiding this comment.
Yes I prefer self-descriptive code.
| this._frame.height = this._bitmap.height; | ||
| Sprite.prototype._onBitmapLoad = function(bitmapLoaded) { | ||
| if(bitmapLoaded === this._bitmap){ | ||
| if (this._refreshFrame && this._bitmap) { |
There was a problem hiding this comment.
I think this._bitmap is always non-null because it equals bitmapLoaded.
So, it's enough if (this._refreshFrame)
There was a problem hiding this comment.
Nope onBitmapLoad is async. there are some chance bitmapLoaded !== this._bitmap
There was a problem hiding this comment.
Oh sorry, I talked about line 230 only.
I thought && this._bitmap isn't necessary because it's always non-null.
There was a problem hiding this comment.
If We approve your suggestion, The reader must know about behavior of bitmapLoaded.
ghost
merged commit 1 participant
This fix bug wrote in Title.