GameLab: Add horizontal and vertical stretch for animations#9546
Conversation
| if (pInst._angleMode === pInst.RADIANS) { | ||
| // This line differs from p5.play upstream because GameLab users | ||
| // should be able to change the width and height of their animations. | ||
| scale(this.scale*dirX*this._horizontalStretch, this.scale*dirY*this._verticalStretch); |
There was a problem hiding this comment.
Instead of adding the _horizontalStretch and _verticalStretch properties in p5.play.js, can we just track these in GameLabSprite (the only place we modify them)? Then if we extract the transform methods here to a helper, and override the helper from GameLabSprite, we should be able to swap in our behavior change.
Sorry if you were already working on this, I know this might be a WIP PR.
There was a problem hiding this comment.
I don't love the names for modifyScaleX/Y, which are named as verbs but in practice are getters with no side effects. They should also probably be private-named (leading underscore) and have a descriptive comment.
More generally, I wonder if we want to create just one method that we'll override, something like this._applySpriteScaleForRendering. I can't think of a situation where we'd want to override the horizontal scale factor but not the vertical one; and even if we did, it'd be pretty simple to do so with the single override method.
21090cb to
5c4f726
Compare
5c4f726 to
a423ddf
Compare
|
@islemaster this is ready now - just need to finish the test so I can add a PR to p5.play too. |
| scale(this.scale*dirX, this.scale*dirY); | ||
| if (pInst._angleMode === pInst.RADIANS) { | ||
| scale(this._getScaleX()*dirX, this._getScaleY()*dirY); | ||
| if (pInst._angleMode == pInst.RADIANS) { |
There was a problem hiding this comment.
Why the switch to soft equality here?
There was a problem hiding this comment.
unintentional - good catch! fixed.
|
LGTM! |
906f3ad Merge pull request #9559 from code-dot-org/topInstructions-integrate-hints (Elijah Hamovitz) 40e882e Merge pull request #9546 from code-dot-org/animation-sizes (Caley Brock) b3a9bbb Merge pull request #9660 from code-dot-org/pd-workshop-join-section (Andrew Oberhardt) ea5c048 Merge branch 'staging' into topInstructions-integrate-hints (Elijah Hamovitz) de6955d missing semicolon fix (Caley Brock) 6bc0459 PR feedback: renamed unit test variables and added comment (Andrew Oberhardt) 85f27bb code review: fix typo (Caley Brock) 2653697 staging content changes (-Ram) (Continuous Integration) f0fcf5c Remove not needed comment (Caley Brock) 8fb23f7 Merge pull request #9675 from code-dot-org/levelbuilder (Ram Kandasamy) ad499a1 Modify p5play to initialize sprite height and width and not change them during update (Caley Brock) 79e2eff levelbuilder content changes (-Ram) (Continuous Integration) 851caa6 Merge pull request #9620 from code-dot-org/photo-credit (Tanya Parker) 0e067b3 Merge pull request #9661 from code-dot-org/peer_review_in_course_ui (Mehal Shah) df67e16 I am annoyed that this works (Mehal Shah) ac5bcd1 staging content changes (-Ram) (Continuous Integration) 6043087 Automatically built. (Continuous Integration) c43f297 remove custom scroll handler, instead hide native scrollbar (Elijah Hamovitz)
For some reason this shows up in code-dot-org/code-dot-org#9546 but not in its matching upstream change https://github.com/molleindustria/p5.play/pull/103. It's necessary for code-dot-org's wrapper.
Add the ability to stretch an animation.
This exposes another known bug with animation width and height: it takes 3 times in the draw loop for width/height to properly set. If we had set width/height outside of the draw loop, the draw loop would have over-ridden our set width and height with the image width and height.
I ran into some weird cases when combining
widthandheightwith scale for animations, but I fixed these in p5.play. They aren't technically related, but these bug fixes make this feature usable:internalWidthandinternalHeightimmediately instead of waiting for the draw loopwidthorheighton an animation it adjusts the size of it's collider