Add support for p5 instance mode #22

toolness · 2016-01-31T17:59:07Z

This adds support for p5 instance mode in p5.play, which fixes #12.

Global Sketch Properties

A number of variables, like allSprites, were formerly global, but now need to be bound to the lifetime of a particular sketch instance. I found doing this a bit tricky because there seems to be no easy way to initialize such properties upon sketch initialization; I've documented this limitation and my workaround for it at processing/p5.js#1263.

Constructor Changes

One particular challenge of this feature is the fact that almost all of the p5.play classes, with the notable exception of Group, now need to be bound to a p5 instance; I've changed their constructors to take this instance as their first argument. However, this makes it a bit tricky to support legacy code.

Ideally, we should simply expose the instantiation of these classes via factory functions on p5.prototype, which is already being done with createSprite, loadAnimation, and loadSpriteSheet--however, the constructors of all these classes are also publicly documented, which means that it's entirely possible for legacy code to be instantiating them directly--for example, the examples/sprites_with_sheet.js example directly calls new SpriteSheet() at one point instead of using loadSpriteSheet().

An alternative here is to simply not support legacy code when it's problematic, and increment the major version number in the next release as per semantic versioning to indicate backwards-incompatible API changes.

As per @islemaster's suggestion, we're exposing curried versions of the constructors on all created p5 instances, so legacy code still works!

Formerly Global Variables

In order for this to work properly, I had to enclose the entire library in an IIFE. Previously, only about half of the library was enclosed in one, which meant that some of its private implementation functions, like construct, were actually in global scope (see below for a full list). While all the examples in the examples/ directory still work fine, I hope that third-party code still works. Might be a good idea to QA this somehow.

The following were formerly globals but are now enclosed in an IIFE:

Sprite (now exposed via p5.prototype.Sprite)
Camera (now exposed via p5.prototype.Camera)
cameraPush
cameraPop
Group (now exposed via p5.prototype.Group)
CircleCollider
AABB
Animation (now exposed via p5.prototype.Animation)
SpriteSheet (now exposed via p5.prototype.SpriteSheet)
Quadtree
updateTree
construct

Note that even for the items that are now exposed via p5.prototype, they won't become globals until and unless p5 enters global mode on page load.

Examples Changes

Nothing was changed in the examples, because I wanted to make sure p5.play still works with legacy code. ~~However, I did get rid of the extra copy of p5.play.js in f1363d1 so that we don't need to pollute the repository's commit history by constantly updating two separate copies of the library.~~ This is still true, but has been spun out into #36.

Other Notes

Of great help here was:

jshint lib/p5.play.js | grep "is not defined"

This logs any undefined globals, which was useful in finding p5 global methods/properties that needed to be bound to p5 instances.

To Do

Add more tests.
Consider exposing Animation, at least in a deprecated way, as its constructor is currently publicly documented.
Consider exposing Camera, at least in a deprecated way, as its constructor is currently publicly documented.
Consider exposing Sprite, at least in a deprecated way, as its constructor is currently publicly documented.
Add/remove/update documentation to reflect any changes in API ~~(deprecated methods, pInst as first argument for constructors, etc)~~.

Conflicts: lib/p5.play.js

toolness · 2016-02-17T13:21:11Z

Hey @islemaster, would you be interested in code reviewing this at all? It'd be really great to have a second pair of eyes on this, but no worries if not.

islemaster · 2016-02-17T13:31:26Z

I'll be happy to. I can take a look later today.
On Feb 17, 2016 5:21 AM, "Atul Varma" notifications@github.com wrote:

Hey @islemaster https://github.com/islemaster, would you be interested
in code reviewing this at all? It'd be really great to have a second pair
of eyes on this, but no worries if not.

—
Reply to this email directly or view it on GitHub
https://github.com/molleindustria/p5.play/pull/22#issuecomment-185202620
.

toolness · 2016-02-17T13:33:14Z

Awesome, thanks!!! I just updated the description of this PR to reflect some API design considerations too.

toolness · 2016-02-17T13:59:46Z

lib/p5.play.js

+// The above will create functions createVector and loadImage, which can be
+// used similar to p5 global mode--however, they're bound to specific p5
+// instances, and can thus be used outside of global mode.
+function createPInstBinder(pInst) {


I'm not sure how I feel about this createPInstBinder thing--I admit that I did it partly out of laziness, but also because I didn't want to hamper readability by having a million things be prefixed with pInst. or this..

islemaster · 2016-02-17T15:30:13Z

A couple of initial thoughts, more on the way.

There's a lot going on here, and a few improvements that look like they could be extracted and merged as separate PRs first. It could help us reason cleanly about this change. Candidates:
- Add .jshintrc and squelch jshint 'is not defined' errors for window
- Add sinon.js and use sinon.stub() in keycodes tests
- Maybe Upgrade p5.js from 0.4.14 to 0.4.21? Not sure from your comments what the motivation was behind the upgrade.
For all the constructors you're deprecating, did you consider exposing curried versions on the instance that use the original API? That way client code could just update from new Group() to new p5instance.Group() (instead of new Group(p5Instance)).

Thanks for tackling this big change!

toolness · 2016-02-17T15:58:46Z

Great ideas! I've just filed those PRs, and will implement your suggestion for the curried constructors.

toolness · 2016-02-17T15:59:50Z

Oh, do you have time to take a look at the new PRs? It's ok if not, just let me know.

islemaster · 2016-02-17T16:08:09Z

I do ; sorry, I'm on Pacific time and can review more in about 90 minutes.
On Feb 17, 2016 7:59 AM, "Atul Varma" notifications@github.com wrote:

Oh, do you have time to take a look at the new PRs? It's ok if not, just
let me know.

—
Reply to this email directly or view it on GitHub
https://github.com/molleindustria/p5.play/pull/22#issuecomment-185270926
.

toolness · 2016-02-17T17:10:48Z

Awesome, thanks!

So in 768b414 I added the currying you suggested.

Now I think all that's left is to decide whether we also want to expose the non-documented constructors CircleCollider, AABB, and Quadtree, and possibly add more unit tests.

toolness · 2016-02-17T17:18:00Z

lib/p5.play.js

+// This returns a factory function, suitable for passing to
+// defineLazyP5Property, that returns a subclass of the given
+// constructor that is always bound to a particular p5 instance.
+function boundConstructorFactory(constructor) {


Ug, it's a factory for creating constructors... Not a huge fan of the multiple levels of indirection but it's all I could think of for now.

Conflicts: test/unit/keycodes.js

islemaster · 2016-02-18T20:53:33Z

lib/p5.play.js

+
+defineLazyP5Property('allSprites', function() { return new Group(); });
+
+p5.prototype.spriteUpdate = true;


Isn't this a problem - the whole reason you wrote defineLazyP5Property? I assume the value of spriteUpdate isn't shared across all p5 instances. If this is working, why can't you just

p5.prototype.allSprites = new Group():

since Group has no dependencies on p5 either? (Probably because you interact with allSprites by modifying it, never overwriting the reference, right?)

It might be easiest to answer these questions by writing some tests that verify these properties are not shared when you make multiple instances.

Oh good idea about the tests! I'll do that.

So the main reason I wrote defineLazyP5Property was actually so that mutable properties, like allSprites, would be per-instance, rather than global. e.g. so that calling sketch1.allSprites.add(mySprite) wouldn't also add the sprite to sketch2.allSprites. Since booleans like spriteUpdate are immutable, and since assigning to the property will actually set it on a particular p5 instance rather than the prototype, I thought it would be OK to put the "starting" value on the prototype. Does that make sense?

In any case, I totally agree that tests would serve both as good executable documentation and as protection against regressions, so I'll do that!

Yep, that make sense now. Thanks for clarifying!

islemaster · 2016-02-19T19:41:50Z

Recommendation: Also break the following commit out into its own PR:

f1363d1 - repo now contains a single version of p5.play.js.

toolness · 2016-02-20T23:50:44Z

Good idea about splitting f1363d1 into its own PR, I will do that!

Update: Just filed #36 for this.

islemaster · 2016-02-22T19:35:19Z

After all that, I don't think I have anything else to add. Thanks for your patience with my gradual understanding of the initialization challenges around p5 instance properties!

@islemaster

As suggeseted by @islemaster in: https://github.com/molleindustria/p5.play/pull/22#discussion_r53377780 Coincidentally, this also gets rid of a nasty bug surfaced by the last merge. Yay!

toolness · 2016-02-24T03:20:16Z

Awesome, thanks @islemaster! And no worries, I'm actually learning how this code works myself too, hehe. It's been very helpful to have a second set of eyes reading over this.

Add support for p5 instance mode

Added a global !mouseIsPressed in addition to !this.mouseIsPressed because when you attempt to drag a sprite you are likely to roll out of its collider setting this.mouseIsPressed to false and triggering the release function. I hope it doesn't break anything.

toolness added 17 commits January 31, 2016 12:27

Make events support p5 instance mode.

3fc6fa8

Sprite, Camera, CircleCollider, AABB support instance mode.

5d580e8

replace _initP5Play w/ defineLazyP5Property.

d32278c

Make updateTree() work in instance mode.

102e593

Add .jshintrc.

8cfb08b

Merge branch 'master' into instance-mode

88e99bc

Conflicts: lib/p5.play.js

Make animations and spritesheets work, fix trig functions.

7984409

Deprecate SpriteSheet() being called from client code.

521fad6

Add some docs and rearrange code a bit.

fd3d9b0

Merge branch 'master' into instance-mode

83ad05b

Merge branch 'master' into instance-mode

675e01e

Conflicts: lib/p5.play.js

Add sinon.js.

9a94619

use sinon.stub() in keycodes tests.

14574da

Add basic test for Spritesheet warnings.

cd9b3d5

minor test readability improvement.

ffe1f80

Upgrade p5.js from 0.4.14 to 0.4.21.

cb6803d

Make tests use p5 instance mode.

2b6bfc7

toolness mentioned this pull request Feb 17, 2016

No way for add-on libraries to be notified of p5 instance construction processing/p5.js#1263

Closed

toolness added 2 commits February 17, 2016 07:54

Add implementation note for defineLazyP5Property.

3a879f9

squelch jshint 'is not defined' errors for window.

22d4106

repo now contains a single version of p5.play.js.

f1363d1

toolness reviewed Feb 17, 2016
View reviewed changes

This was referenced Feb 17, 2016

Upgrade p5.js from 0.4.14 to 0.4.21. #33

Merged

Use sinon.js for stubbing and spying in tests #34

Merged

Add support for jshint #35

Merged

Attach bound constructors to p5 instances.

768b414

toolness reviewed Feb 17, 2016
View reviewed changes

toolness added 3 commits February 17, 2016 14:54

Fix typo that made collisions.js example break.

125ffe9

Merge branch 'master' into instance-mode

2586772

Conflicts: test/unit/keycodes.js

Merge branch 'master' into instance-mode

c511663

islemaster reviewed Feb 18, 2016
View reviewed changes

toolness added 2 commits February 21, 2016 08:08

Add test/unit/sketch-properties.js.

2b115bf

Add test for spriteUpdate property.

90e06cf

toolness added 2 commits February 23, 2016 22:00

Merge branch 'master' into instance-mode

e2566c8

Simplify defineLazyP5Property implementation.

8a56187

As suggeseted by @islemaster in: https://github.com/molleindustria/p5.play/pull/22#discussion_r53377780 Coincidentally, this also gets rid of a nasty bug surfaced by the last merge. Yay!

toolness changed the title ~~[WIP] Add support for p5 instance mode~~ Add support for p5 instance mode Feb 24, 2016

toolness added a commit that referenced this pull request Feb 24, 2016

Merge pull request #22 from toolness/instance-mode

e3e2375

Add support for p5 instance mode

toolness merged commit e3e2375 into quinton-ashley:master Feb 24, 2016

toolness mentioned this pull request Feb 24, 2016

Deduplicate key- and mouse-state checks #28

Merged

toolness mentioned this pull request Mar 14, 2016

Use 'init' registerMethod() instead of lazily-setting properties #46

Closed

This was referenced Mar 30, 2016

Document how to extend/subclass Sprite class #71

Closed

"static" methods are documented in a confusing way processing/p5.js#1336

Closed

GoToLoop mentioned this pull request Jan 3, 2020

Can't use modern ES6 inheritance #185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for p5 instance mode #22

Add support for p5 instance mode #22

toolness commented Jan 31, 2016

toolness commented Feb 17, 2016

islemaster commented Feb 17, 2016

toolness commented Feb 17, 2016

toolness Feb 17, 2016

islemaster commented Feb 17, 2016

toolness commented Feb 17, 2016

toolness commented Feb 17, 2016

islemaster commented Feb 17, 2016

toolness commented Feb 17, 2016

toolness Feb 17, 2016

islemaster Feb 18, 2016

islemaster Feb 18, 2016

toolness Feb 20, 2016

islemaster Feb 22, 2016

islemaster commented Feb 19, 2016

toolness commented Feb 20, 2016

islemaster commented Feb 22, 2016

toolness commented Feb 24, 2016


		defineLazyP5Property('allSprites', function() { return new Group(); });

		p5.prototype.spriteUpdate = true;

Add support for p5 instance mode #22

Add support for p5 instance mode #22

Conversation

toolness commented Jan 31, 2016

Global Sketch Properties

Constructor Changes

Formerly Global Variables

Examples Changes

Other Notes

To Do

toolness commented Feb 17, 2016

islemaster commented Feb 17, 2016

toolness commented Feb 17, 2016

toolness Feb 17, 2016

Choose a reason for hiding this comment

islemaster commented Feb 17, 2016

toolness commented Feb 17, 2016

toolness commented Feb 17, 2016

islemaster commented Feb 17, 2016

toolness commented Feb 17, 2016

toolness Feb 17, 2016

Choose a reason for hiding this comment

islemaster Feb 18, 2016

Choose a reason for hiding this comment

islemaster Feb 18, 2016

Choose a reason for hiding this comment

toolness Feb 20, 2016

Choose a reason for hiding this comment

islemaster Feb 22, 2016

Choose a reason for hiding this comment

islemaster commented Feb 19, 2016

toolness commented Feb 20, 2016

islemaster commented Feb 22, 2016

toolness commented Feb 24, 2016