Add entities pool #1954

dmarcos · 2016-09-30T21:37:22Z

No description provided.

fernandojsg · 2016-09-30T22:10:55Z

examples/test/pool/components/spaceship.js

+    var laserEl;
+    var position;
+    if (evt.keyCode !== 32) { return; }
+    laserEl = el.sceneEl.components.pool.requestEntity();


What about requestEntity('laser')?

what does that mean?

if you want to have multiple pools you add multiple pool components and call requestEntity on them:
<a-scene pool__laser="mixin: laser; size: 10" pool__bomb="mixin: bomb; size: 10"></a-scene>

sceneEl.components.pool__laser.requestEntity()

aps sorry I didn't notice that pool was the name of the component and not a global that will store all the pools. I mistook this use case with accessing directly the system.

fernandojsg · 2016-09-30T22:21:49Z

src/components/scene/pool.js

+  schema: {
+    mixin: {default: ''},
+    size: {default: 0}
+  },


Add another parameter to force the requestEntity to return a new entity even if there's no available entity on the pool, it will create a new one and pushed to the pool and return it

fernandojsg · 2016-09-30T22:23:06Z

src/components/scene/pool.js

+    this.pool = [];
+    this.pooledEls = [];
+    for (var i = 0; i < this.data.size; ++i) {
+      el = document.createElement('a-entity');


Takes this part out from the loop to a another function createEntity/pushNewEntity/..., so it could be reused on requestEntity when the previous described param is set to true and there's no available entity

ngokevin

Remember documentation once everything's ready to go.

ngokevin · 2016-10-01T04:23:20Z

src/components/scene/pool.js

+ * @member {array} pooledEls - Entities of the pool in use.
+ *
+ */
+module.exports.Component = register('pool', {


Common stuff I see online refer to it as "Object Pooling" so consider entity-pool to be more clear. I might've not immediately understood what a pool component did, but entity-pool I might've.

When an application requires pools I expect to often see multiple instances pool__bullets, pool__enemies... If we rename pool to entity-pool you will end up with entity-pool__enemies , entity-pool_bullets, Is not that to verbose? @fernandojsg

Entity pool is more clear than just pool but due its nature, we'll have plenty of instances on a-scene therefore we'll have entity-pool__whatever as @dmarcos said and it looks quite ugly, so I would choose to use just pool

ngokevin · 2016-10-01T04:23:42Z

examples/test/pool/components/laser.js

+
+  init: function () {
+    var el = this.el;
+    var geometry = 'primitive: box; height: 2; width: 0.1; depth: 0.1';


Intermediate vars necessary?

It makes lines shorter. Easier to read

ngokevin · 2016-10-01T04:24:58Z

src/components/scene/pool.js

@@ -0,0 +1,103 @@
+var debug = require('../../utils/debug');
+var register = require('../../core/component').registerComponent;


s/register/registerComponent

ngokevin · 2016-10-01T04:25:21Z

src/components/scene/pool.js

+  schema: {
+    mixin: {default: ''},
+    size: {default: 0},
+    dynamicSize: {default: false}


dynamicallySize might differentiate it as a boolean. dynamicSize sounds like it'd take a size.

dynamicallySize is a mouthful. Adverbs should be banned from earth.

I agree with @ngokevin that dynamicSize sounds like it expects a numeric value instead of boolean, but I don't like dynamicallySize either, what about just dynamic?

... and to be correct you would have to pair it with a participle: dynamicallySized

ngokevin · 2016-10-01T04:26:11Z

src/components/scene/pool.js

+    }
+  },
+
+  update: function (oldData) {


i like all lifecycle handlers on top in order they're invoked, and then internal methods

Where is that stated? Is not a policy difficult to understand, enforce and context dependent? Sometimes we sort things alphabetically and sometimes we don't. I recently realized that the order of variables or methods in a file does not help me much. On the other hand it introduces friction and sometimes you have to do weird contortions to conform to the policy like for instance when sorting the variables alphabetically.

ngokevin · 2016-10-01T04:32:15Z

src/components/scene/pool.js

+    var pooledEls = this.pooledEls;
+    return function () {
+      if (pooledEls.indexOf(this) === -1) { return; }
+      playMethod.call(this);


This line doesn't have test coverage

I would not be too anal about 100% coverage

Small test to prevent potential bugs, good trade off (we've had plenty bugs that could've been caught with tests). At least test the below case of dynamically fetching from any empty pool.

ngokevin · 2016-10-01T04:32:21Z

src/components/scene/pool.js

+        warn('Requested entity from empty pool ' + this.name);
+        return;
+      }
+      this.createEntity();


This line doesn't have test coverage

ngokevin · 2016-10-01T04:32:50Z

src/components/scene/pool.js

+  /**
+   * Used to return a used entity to the pool
+   */
+  returnEntity: function (el) {


Maybe releaseEntity? returnEntity sounds like it'll return an entity

It actually returns the entity to the pool. Release is a term used in memory allocation / deallocation which is the opposite idea of using a pool

ngokevin · 2016-10-01T04:37:18Z

tests/components/scene/pool.test.js

+
+suite('pool', function () {
+  setup(function (done) {
+    var el = this.sceneEl = document.createElement('a-scene');


You can do:

var sceneEl = helpers.entityFactory().sceneEl; var mixinEl = helpers.mixinFactory('test', {material: 'color: red'}, sceneEl);

This will create <a-assets> and <a-mixin> for you.

ngokevin · 2016-10-01T04:38:37Z

examples/test/pool/index.html

+    var sceneEl = document.querySelector('a-scene');
+    var message = '<p>Left and right arrows to move and space bar to shoot</p><p>Used Pool Entities ';
+    sceneEl.addEventListener('loaded', updateMessage);
+    function updateMessage() {


I think we should encourage a component here:

Don't have to wait for the scene to load. Lifecycle is handled for you

Don't have to create your own RAF.

I prefer to see this right there on the index.html and not hidden in a component.

You can still define it in the html file

If you want it, it's a bit cleaner:

var message = 'Left and right arrows to move and space bar to shootUsed Pool Entities '; AFRAME.registerComponent('update-pool-message', { schema: {type: 'selector'}, tick: function () { var poolComponent = this.el.components.pool; var poolSize = poolComponent.availableEls.length + poolComponent.usedEls.length; var usedPoolEntities = this.el.components.pool.usedEls.length; this.data.innerHTML = message + usedPoolEntities + '/' + poolSize + ''; } });

fernandojsg · 2016-10-04T10:12:40Z

docs/components/pool.md

+|----------|--------------------------------------------------------------------------------------|---------------|
+| mixin     | Mixin used to initialize the entities of the pool.                          | ''        |
+| size    | the number of preallocated entities in the pool | 0          |
+| dynamicSize | the pool grows automatically if more entities are requested after reaching the current size | false |


dynamic instead of dynamicSize

fernandojsg · 2016-10-04T11:10:39Z

src/components/scene/pool.js

+    el = this.availableEls.shift();
+    this.usedEls.push(el);
+    el.setAttribute('visible', true);
+    return el;


I believe we should call el.play() by default before returning the entity as it would be the most common case, isn't it?

fernandojsg · 2016-10-04T11:11:30Z

src/components/scene/pool.js

+        warn('Requested entity from empty pool ' + this.name);
+        return;
+      }
+      this.createEntity();


What about insert a warning message anyway even if dynamic=true so the user could adjust the pool size better?

fernandojsg · 2016-10-04T11:29:22Z

src/components/scene/pool.js

+   */
+  createEntity: function () {
+    var el = document.createElement('a-entity');
+    el.play = this.wrapPlay(el.play);


What about adding el.pool = this; so each entity could have a reference of its pool, and for example when a bullet collides with an object it could do this.pool.returnEntity(this) without need to store a reference to the pool by hand

fernandojsg · 2016-10-05T00:00:23Z

It's looking good to me, @ngokevin ?

ngokevin

r+wc

ngokevin · 2016-10-05T00:11:37Z

src/components/scene/pool.js

+ * in a game where you want to reuse enemies entities.
+ *
+ * @member {array} availableEls - Available entities in the pool.
+ * @member {array} useedEls - Entities of the pool in use.


ngokevin · 2016-10-05T00:11:57Z

docs/components/pool.md

+<a-scene pool="mixin: enemy; size : 10"></a-scene>
+```
+
+## Properties


should probably document the methods as well since those are part of the API

ngokevin · 2016-10-05T00:14:11Z

examples/test/pool/index.html

+    var sceneEl = document.querySelector('a-scene');
+    var message = '<p>Left and right arrows to move and space bar to shoot</p><p>Used Pool Entities ';
+    sceneEl.addEventListener('loaded', updateMessage);
+    function updateMessage() {


If you want it, it's a bit cleaner:

var message = 'Left and right arrows to move and space bar to shootUsed Pool Entities '; AFRAME.registerComponent('update-pool-message', { schema: {type: 'selector'}, tick: function () { var poolComponent = this.el.components.pool; var poolSize = poolComponent.availableEls.length + poolComponent.usedEls.length; var usedPoolEntities = this.el.components.pool.usedEls.length; this.data.innerHTML = message + usedPoolEntities + '/' + poolSize + ''; } });

ngokevin · 2016-10-05T00:15:48Z

tests/components/scene/pool.test.js

+  setup(function (done) {
+    var el = helpers.entityFactory();
+    var sceneEl = this.sceneEl = el.parentNode;
+    sceneEl.setAttribute('pool', 'mixin: test; size: 1');


test dynamic?

dmarcos · 2016-10-05T05:01:43Z

I added a test for a dynamic pool and made the example screen message a component. I'm merging this....

dmarcos added the work in progress label Sep 30, 2016

fernandojsg reviewed Sep 30, 2016

View reviewed changes

dmarcos added needs review and removed work in progress labels Oct 1, 2016

ngokevin requested changes Oct 1, 2016

View reviewed changes

ngokevin added needs changes and removed needs review labels Oct 1, 2016

dmarcos added 7 commits October 3, 2016 16:49

Add entities pool

d090078

Add dynamic flag

769f5d7

add tests

7251497

add docs

9c245e1

change name private variables

41f7823

Changing order of nexttick

6fafad6

Simplify test setup

5c99220

dmarcos force-pushed the pool branch from 4bdf65d to 5c99220 Compare October 4, 2016 00:54

clean up

eb2ed52

dmarcos added needs review and removed needs changes labels Oct 4, 2016

fernandojsg added needs changes and removed needs review labels Oct 4, 2016

fernandojsg reviewed Oct 4, 2016

View reviewed changes

address review comments

e94479b

dmarcos added needs review and removed needs changes labels Oct 4, 2016

ngokevin approved these changes Oct 5, 2016

View reviewed changes

more review comments

986d81a

dmarcos merged commit 1b3da9b into aframevr:master Oct 5, 2016

		@@ -0,0 +1,103 @@
		var debug = require('../../utils/debug');
		var register = require('../../core/component').registerComponent;

Add entities pool #1954

Add entities pool #1954

Conversation

dmarcos commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngokevin left a comment

Choose a reason for hiding this comment

ngokevin Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngokevin Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

fernandojsg Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandojsg commented Oct 5, 2016

ngokevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngokevin Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos commented Oct 5, 2016

dmarcos Sep 30, 2016 •

edited

Loading

ngokevin Oct 1, 2016 •

edited

Loading

dmarcos Oct 1, 2016 •

edited

Loading

dmarcos Oct 1, 2016 •

edited

Loading

ngokevin Oct 5, 2016 •

edited

Loading

fernandojsg Oct 4, 2016 •

edited

Loading

ngokevin Oct 5, 2016 •

edited

Loading