[UEPR-312] Clean up face sensing codebase #303
Conversation
- Delete commented out and unused code - Remove unused properties from FACE_SENSING_STATE - Mark private methods and properties with underscore for consistency - Add method/propery docs - Extract some generic methods to utils.js - Other minor improvements
| /** | ||
| * Determine if the drawable is touching a rectangle. | ||
| * | ||
| * @param {int} drawableID The ID of the drawable to check. | ||
| * @param {int} left - The left X coordinate of the rectangle. | ||
| * @param {int} top - The top Y coordinate of the rectangle. | ||
| * @param {int} right - The right X coordinate of the rectangle. | ||
| * @param {int} bottom - The bottom Y coordinate of the rectangle. | ||
| * @returns {boolean} If the drawable has any pixels that would draw in the rectangle area | ||
| */ |
There was a problem hiding this comment.
😎
If you run across missing or incorrect JSDoc that's making your life difficult, please feel very free to fix it up. Some of the JSDoc in here, especially in the VM and renderer, was written in the early days of JSDoc and (more importantly) the early days of our (at least my) understanding of JSDoc and JavaScript in general, so some of it is wrong or invalid, and a lot of it is just missing.
TL;DR: Thank you!
There was a problem hiding this comment.
(I'd like to move to TS when and where it makes sense, but that's a long walk...)
| @@ -232,7 +264,6 @@ class Scratch3FaceSensingBlocks { | |||
| blockType: BlockType.COMMAND, | |||
| filter: [TargetType.SPRITE] | |||
| }, | |||
| '---', | |||
There was a problem hiding this comment.
Were these separators intentionally removed? It seems OK to me; just double-checking.
There was a problem hiding this comment.
The separators were there by design and should not be removed - please put them back
| x: position.x - 240, | ||
| y: 180 - position.y |
| @@ -756,7 +756,7 @@ class RenderedTarget extends Target { | |||
| * Return whether touching a point. | |||
| * @param {number} x X coordinate of test point. | |||
| * @param {number} y Y coordinate of test point. | |||
| * @return {boolean} True iff the rendered target is touching the point. | |||
| * @return {boolean} True if the rendered target is touching the point. | |||
There was a problem hiding this comment.
I'm OK with these changes, but just as an FYI, I think in each of these cases the "iff" was intended as an abbreviation for "if and only if" rather than being a typo. On the other hand, that's excessively fancy and maybe not even strictly true. Also—and this may be the strongest argument in favor of this change—red squiggly lines are annoying. :D
|
While we're fixing comments - in the extension index.js line 28 |
|
This is great so far - there are actually a few additional bits of code that can be removed. They are all related to the idea of "attachment," which I prototyped but decided not to use. If you're interested, I talk about the different ideas we tried, including an "attach" block, and how we settled on the final design, in this talk from last year's Blockly summit. The stop button event is wired up to |
adzhindzhi
merged commit 257dc82
into
scratchfoundation:UEPR-282-face-sensing
Resolves
UEPR-312
Proposed Changes
Reason for Changes
Get the face sensing code ready for production