Move Skin functionality out of Drawable into its own classes#67
Move Skin functionality out of Drawable into its own classes#67cwillisf merged 6 commits intoscratchfoundation:developfrom
Skin functionality out of Drawable into its own classes#67Conversation
This constant will soon be shared with another class.
There is now a `Skin` base class along with `SVGSkin` and `BitmapSkin` classes for vector and bitmap skin support. Loading a skin by URL is in the process of being deprecated. In the future skin data should be downloaded outside the renderer and supplied through new `create*Skin` API calls.
- Add & correct a few comments - Make `Drawable` IDs work like `Skin` IDs - Remove some dead code - Fix costume resolution in `BitmapSkin` before first `setBitmap` call - Rename `Drawable` & `Skin` management variables in `RenderWebGL` to clarify their purpose and better distinguish them from each other - Remove problematic premature optimization in `_drawThese` - Fix missing return statement in `Skin`'s `setRotationCenter` - Fix mismatch between getTexture & getUniforms in the `Skin` class
This allows any `Drawable` using the `Skin` to update its transform as necessary.
- `Drawable` now removes its `Skin` on `dispose`, disconnecting events. - Creating a new `Drawable` doesn't always create a new `Skin` any more: now it can share an existing skin. - Don't throw when asked to update the properties of a `Drawable` that has already been destroyed. This seems like a VM bug but was causing overwhelming output in the browser for some projects.
rschamp
left a comment
rschamp
left a comment
There was a problem hiding this comment.
Something that stands out is the reference to the renderer in the Skins — is that a necessary evil? I would expect to see the renderer have references to the skins and then do the things it needs to with them rather than have renderer methods called by the skins "magically". Previously it had a reference to gl — was that any better or worse than the renderer itself?
|
Hmm... I was thinking that I made it so they take a reference to the renderer for consistency with Drawable, but it appears that I never committed a version of Drawable that takes a reference to the renderer. Heh. Taking just |
|
Oh - I found the reason. Hmm... My upcoming pen code includes the following changes:
Specifically, @rschamp thoughts? Would you prefer a separate event emitter rather than emitting from the renderer itself? Some other way to handle resize notification? |
|
In the case of the renderer emitting resize events which But I think since this is dependent on the other work you're doing, we should keep it as-is and refactor later — that sort of thing won't be hard to change. |
thisandagain
left a comment
thisandagain
left a comment
There was a problem hiding this comment.
This looks really clean. Thanks @cwillisf.
| if (!willCallCallback) { | ||
| callback(null, this._pendingSkin, source); | ||
| } | ||
| getEnabledEffects () { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
3 participants
Moving
Skinfunctionality (corresponding to costumes) out ofDrawable(corresponding to sprites & clones) allows multipleDrawableinstances to share the sameSkinand allows aDrawableto change betweenSkins without loading the image fresh each time.In addition, this change moves management of
DrawableIDs out ofDrawableand into the renderer, which means thatDrawableno longer has a "split personality" (static vs. instance). It also makes debugging easier: if you can access the renderer you can inspect all relevant state.Setting a
Skinfrom a URL has been improved dramatically by this change, but really the renderer shouldn't be responsible for downloading and parsing image files. The newcreateBitmapSkinandcreateSVGSkinmethods should be used after the renderer's client (the VM, GUI, etc.) obtains an image through a download, the paint editor, etc.This resolves scratchfoundation/scratch-vm#372