Feature fencing

[griffpatch]

Requires the fencing pull requet in "scratch-render" to be merged before merging this one.

[griffpatch]

He he - thought that might come up next. Yes indeed, it was solely for efficiency. I would have in some ways preferred to move the whole thing into the position function, but that still had the requirement of then returning the X & Y back to the vm. If you are sure you prefer the overhead of needing to return a new object rather than pass by reference then we could merge the functions perhaps instead. I'll take a look when I get a chance to review what I did. On 13 Feb 2017 6:22 p.m., "Ray Schamp" <[email protected]> wrote: *@rschamp* commented on this pull request. ------------------------------ In src/sprites/rendered-target.js <#452 (review)>:

@@ -170,6 +170,7 @@ RenderedTarget.prototype.setXY = function (x, y) {

this.x = x; this.y = y; if (this.renderer) { + this.renderer.fencePositionOfDrawable(this.drawableID, this); If they can be avoided, I stray away from functions that mutate their arguments both because they are easier to test, and because it's more obvious how they work when you come across them. For instance here it's possibly that more than x and y are changing, but you would have to go check the renderer to find out. Is there a performance reason you chose not to pass in [x, y] and have this method return [newX, newY]? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#452 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGbNvjxxbBpNe3K7lZf-QEoTEvPQGR7Gks5rcJ91gaJpZM4L-MMe> .

[griffpatch]

Right, here is a refactored approach that appears to not have a huge impact on speed. I think you'll prefer this way of doing things?

[rschamp]

Yes this looks better to me, thanks!

[cwillisf]

Looks good now -- thanks for doing this!

[griffpatch]

This is so embarrassing. I have sourced the reason for why I was unable to detect the bug in my local version. Since the change to how npm start runs locally my code has no longer been running off the latest changes I make, but off whatever was last built... I've been changing things and my tests still worked fine.

Do I need to raise a new pull request to bring in the correct change?

[rschamp]

Hey @griffpatch — no worries at all! We should have pulled these changes down and tested them in action before merging.

I was going to take a look into what was going on, but if you think you've got it, yes, please make new PRs which include your previous changes plus the fix. Then we'll check it out and test locally.

Thanks for all your work on 3.0!

[griffpatch]

Thanks, I am sorry... My system got in a real pickle since the webpack config got changed and I didn't notice it wasn't reflecting the changes anymore in my test environment. The problem was that I was still assuming an object with an x & y parameter being returned, but I'd switched to an array [x,y]... it was bringing back undefineds for both therefore when reading the x & y... which then set all x & y's to undefined in the scratch-vm.

…

On 15 February 2017 at 15:26, Ray Schamp ***@***.***> wrote: Hey @griffpatch <https://github.com/griffpatch> — no worries at all! We should have pulled these changes down and tested them in action before merging. I was going to take a look into what was going on, but if you think you've got it, yes, please make new PRs which include your previous changes plus the fix. Then we'll check it out and test locally. Thanks for all your work on 3.0! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#452 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGbNvoJmQbkaAEU_gw2gzsxcnUDqF-k5ks5rcxkagaJpZM4L-MMe> .