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.

Sign up for GitHub

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

Add update drawable methods for each property #469

Merged
merged 2 commits into from
Oct 1, 2019
Merged

Add update drawable methods for each property #469

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96aa930
update drawable calls
mzgoddard Jun 5, 2019
8e836a7
Add a bug to the todos for missing drawable
fsih Oct 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 79 additions & 36 deletions src/Drawable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,55 +184,98 @@ class Drawable {
}

/**
* Update the position, direction, scale, or effect properties of this Drawable.
* @param {object.<string,*>} properties The new property values to set.
* Update the position if it is different. Marks the transform as dirty.
* @param {Array.<number>} position A new position.
*/
updateProperties (properties) {
let dirty = false;
if ('position' in properties && (
this._position[0] !== properties.position[0] ||
this._position[1] !== properties.position[1])) {
this._position[0] = Math.round(properties.position[0]);
this._position[1] = Math.round(properties.position[1]);
dirty = true;
}
if ('direction' in properties && this._direction !== properties.direction) {
this._direction = properties.direction;
updatePosition (position) {
if (this._position[0] !== position[0] ||
this._position[1] !== position[1]) {
this._position[0] = Math.round(position[0]);
this._position[1] = Math.round(position[1]);
this.setTransformDirty();
}
}

/**
* Update the direction if it is different. Marks the transform as dirty.
* @param {number} direction A new direction.
*/
updateDirection (direction) {
if (this._direction !== direction) {
this._direction = direction;
this._rotationTransformDirty = true;
dirty = true;
this.setTransformDirty();
}
if ('scale' in properties && (
this._scale[0] !== properties.scale[0] ||
this._scale[1] !== properties.scale[1])) {
this._scale[0] = properties.scale[0];
this._scale[1] = properties.scale[1];
}

/**
* Update the scale if it is different. Marks the transform as dirty.
* @param {Array.<number>} scale A new scale.
*/
updateScale (scale) {
if (this._scale[0] !== scale[0] ||
this._scale[1] !== scale[1]) {
this._scale[0] = scale[0];
this._scale[1] = scale[1];
this._rotationCenterDirty = true;
this._skinScaleDirty = true;
dirty = true;
this.setTransformDirty();
}
if ('visible' in properties) {
this._visible = properties.visible;
}

/**
* Update visibility if it is different. Marks the convex hull as dirty.
* @param {boolean} visible A new visibility state.
*/
updateVisible (visible) {
if (this._visible !== visible) {
this._visible = visible;
this.setConvexHullDirty();
}
if (dirty) {
this.setTransformDirty();
}

/**
* Update an effect. Marks the convex hull as dirty if the effect changes shape.
* @param {string} effectName The name of the effect.
* @param {number} rawValue A new effect value.
*/
updateEffect (effectName, rawValue) {
const effectInfo = ShaderManager.EFFECT_INFO[effectName];
if (rawValue) {
this._effectBits |= effectInfo.mask;
} else {
this._effectBits &= ~effectInfo.mask;
}
const converter = effectInfo.converter;
this._uniforms[effectInfo.uniformName] = converter(rawValue);
if (effectInfo.shapeChanges) {
this.setConvexHullDirty();
}
}

/**
* Update the position, direction, scale, or effect properties of this Drawable.
* @deprecated Use specific update* methods instead.
* @param {object.<string,*>} properties The new property values to set.
*/
updateProperties (properties) {
if ('position' in properties) {
this.updatePosition(properties.position);
}
if ('direction' in properties) {
this.updateDirection(properties.direction);
}
if ('scale' in properties) {
this.updateScale(properties.scale);
}
if ('visible' in properties) {
this.updateVisible(properties.visible);
}
const numEffects = ShaderManager.EFFECTS.length;
for (let index = 0; index < numEffects; ++index) {
const effectName = ShaderManager.EFFECTS[index];
if (effectName in properties) {
const rawValue = properties[effectName];
const effectInfo = ShaderManager.EFFECT_INFO[effectName];
if (rawValue) {
this._effectBits |= effectInfo.mask;
} else {
this._effectBits &= ~effectInfo.mask;
}
const converter = effectInfo.converter;
this._uniforms[effectInfo.uniformName] = converter(rawValue);
if (effectInfo.shapeChanges) {
this.setConvexHullDirty();
}
this.updateEffect(effectName, properties[effectName]);
}
}
}
Expand Down
122 changes: 117 additions & 5 deletions src/RenderWebGL.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1313,9 +1313,122 @@ class RenderWebGL extends EventEmitter {
}, null);
}

/**
* Update a drawable's skin.
* @param {number} drawableID The drawable's id.
* @param {number} skinId The skin to update to.
*/
updateDrawableSkinId (drawableID, skinId) {
const drawable = this._allDrawables[drawableID];
// TODO: vm's requests to drawableID that do not have a Drawable object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this todo comment (and the associated ones below) clearer in terms of what actually needs to be done?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we should file an issue either here or in VM and note it in these todo comments as well as the one in the now deprecated updateDrawableProperties function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

For clarity: The text of this current comment is an abbreviation of the comment in updateDrawable. https://github.com/LLK/scratch-render/pull/469/files#diff-0e66131b35a2396bf8b007193ee7bc77R1437-R1442

    if (!drawable) {
        /**
         * @todo fix whatever's wrong in the VM which causes this, then add a warning or throw here.
         * Right now this happens so much on some projects that a warning or exception here can hang the browser.
         */
         return;

This is a very old issue. And not one these new updateDrawable* methods are introducing.

if (!drawable) return;
drawable.skin = this._allSkins[skinId];
}

/**
* Update a drawable's skin rotation center.
* @param {number} drawableID The drawable's id.
* @param {Array.<number>} rotationCenter The rotation center for the skin.
*/
updateDrawableRotationCenter (drawableID, rotationCenter) {
const drawable = this._allDrawables[drawableID];
// TODO: vm's requests to drawableID that do not have a Drawable object.
if (!drawable) return;
drawable.skin.setRotationCenter(rotationCenter[0], rotationCenter[1]);
}

/**
* Update a drawable's skin and rotation center together.
* @param {number} drawableID The drawable's id.
* @param {number} skinId The skin to update to.
* @param {Array.<number>} rotationCenter The rotation center for the skin.
*/
updateDrawableSkinIdRotationCenter (drawableID, skinId, rotationCenter) {
const drawable = this._allDrawables[drawableID];
// TODO: vm's requests to drawableID that do not have a Drawable object.
if (!drawable) return;
drawable.skin = this._allSkins[skinId];
drawable.skin.setRotationCenter(rotationCenter[0], rotationCenter[1]);
}

/**
* Update a drawable's position.
* @param {number} drawableID The drawable's id.
* @param {Array.<number>} position The new position.
*/
updateDrawablePosition (drawableID, position) {
const drawable = this._allDrawables[drawableID];
// TODO: vm's requests to drawableID that do not have a Drawable object.
if (!drawable) return;
drawable.updatePosition(position);
}

/**
* Update a drawable's direction.
* @param {number} drawableID The drawable's id.
* @param {number} direction A new direction.
*/
updateDrawableDirection (drawableID, direction) {
const drawable = this._allDrawables[drawableID];
// TODO: vm's requests to drawableID that do not have a Drawable object.
if (!drawable) return;
drawable.updateDirection(direction);
}

/**
* Update a drawable's scale.
* @param {number} drawableID The drawable's id.
* @param {Array.<number>} scale A new scale.
*/
updateDrawableScale (drawableID, scale) {
const drawable = this._allDrawables[drawableID];
// TODO: vm's requests to drawableID that do not have a Drawable object.
if (!drawable) return;
drawable.updateScale(scale);
}

/**
* Update a drawable's direction and scale together.
* @param {number} drawableID The drawable's id.
* @param {number} direction A new direction.
* @param {Array.<number>} scale A new scale.
*/
updateDrawableDirectionScale (drawableID, direction, scale) {
const drawable = this._allDrawables[drawableID];
// TODO: vm's requests to drawableID that do not have a Drawable object.
if (!drawable) return;
drawable.updateDirection(direction);
drawable.updateScale(scale);
}

/**
* Update a drawable's visibility.
* @param {number} drawableID The drawable's id.
* @param {boolean} visible Will the drawable be visible?
*/
updateDrawableVisible (drawableID, visible) {
const drawable = this._allDrawables[drawableID];
// TODO: vm's requests to drawableID that do not have a Drawable object.
if (!drawable) return;
drawable.updateVisible(visible);
}

/**
* Update a drawable's visual effect.
* @param {number} drawableID The drawable's id.
* @param {string} effectName The effect to change.
* @param {number} value A new effect value.
*/
updateDrawableEffect (drawableID, effectName, value) {
const drawable = this._allDrawables[drawableID];
// TODO: vm's requests to drawableID that do not have a Drawable object.
if (!drawable) return;
drawable.updateEffect(effectName, value);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are 9 methods which all do basically the same thing but on a different property. It would probably be more DRY to add a getDrawable method to RenderWebGL and then have the VM call the respective update methods directly on the drawable, though that would have the downside of coupling the Drawable's API and class structure to the VM somewhat.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That's a fair thought.

I think making such a change in this PR will be out of scope. We might be repeating ourselves some, but we are not changing the API in a large way.

that would have the downside of coupling the Drawable's API and class structure to the VM

This would be a pretty big change. RenderWebGL is essentially designed as a foreign function interface in case we need it to become one.

Hmm. I think this code is DRY. As each method calls a different property, they are not doing the same thing. There is the drawable safety check but that's unavoidable. It's the nature of checking that a variable has value before operating on it. It doesn't look dry, but I think that's just the limits of the JS syntax.


/**
* Update the position, direction, scale, or effect properties of this Drawable.
* @deprecated Use specific updateDrawable* methods instead.
* @param {int} drawableID The ID of the Drawable to update.
* @param {object.<string,*>} properties The new property values to set.
*/
Expand All @@ -1329,11 +1442,10 @@ class RenderWebGL extends EventEmitter {
return;
}
if ('skinId' in properties) {
drawable.skin = this._allSkins[properties.skinId];
this.updateDrawableSkinId(drawableID, properties.skinId);
}
if ('rotationCenter' in properties) {
const newRotationCenter = properties.rotationCenter;
drawable.skin.setRotationCenter(newRotationCenter[0], newRotationCenter[1]);
this.updateDrawableRotationCenter(drawableID, properties.rotationCenter);
}
drawable.updateProperties(properties);
}
Expand Down Expand Up @@ -1627,14 +1739,14 @@ class RenderWebGL extends EventEmitter {
}

twgl.setUniforms(currentShader, uniforms);

/* adjust blend function for this skin */
if (drawable.skin.hasPremultipliedAlpha){
gl.blendFuncSeparate(gl.ONE, gl.ONE_MINUS_SRC_ALPHA, gl.ONE, gl.ONE_MINUS_SRC_ALPHA);
} else {
gl.blendFuncSeparate(gl.SRC_ALPHA, gl.ONE_MINUS_SRC_ALPHA, gl.ONE, gl.ONE_MINUS_SRC_ALPHA);
}

twgl.drawBufferInfo(gl, this._bufferInfo, gl.TRIANGLES);
}

Expand Down