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.

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

Modularize the server; Add Player and BSTree classes #184

Closed
wants to merge 10 commits into from
Closed

Modularize the server; Add Player and BSTree classes #184

wants to merge 10 commits into from

Conversation

jniles
Copy link
Contributor

@jniles jniles commented Jun 5, 2015

UPDATE
@hunterpl and I found a bug in this PR. I'll let you know when we've written a patch and is safe to merge.
Bug is now fixed. This should be safe to merge.

This PR tries to modularize the server so that we can test it properly. It contains major changes.

Players are now encapsulated in the Player object. It holds player properties and state. When a new player joins, it is instantiated with a call new Player({ /* ... */ });. Players only have two methods currently:

  • player.move(target) moves the player toward the target (derived from legacy movePlayer() function).
  • player.serialize() provides a hook for the developers working on binary serialization of player state. See issue Binary Protocol #177 . It is currently unused, but potentially useful.

Players reside in a PlayerTree, which is an implementation of the Binary Search Tree. Unlike a regular array, it provides guarantees of O(log n) search speed in the best case and O(n) in the worst case. See the file bstree.js for full analysis. In the future, we will want to use a Red-Black Tree, a binary search tree which guarantees a O(log n) time for insertion, removal, and search in the worst case. The API will be identical, so we can transparently drop it in when it is complete.

Tree API:

var BST = require('./path/to/bstree');
var tree = new BST();
var id = 1, data = { name : 'florence' };
tree.insert(id, data);
tree.insert(3, { name : 'jaqueline' });

var users = tree.asArray();
// users is [ { name : 'florence' }, { name : 'jaqueline' }]

tree.remove(1);
console.log(tree.asArray()); // [{ name : 'jaqueline' ]]

console.log(tree.find(3)); // { name : 'jaqueline' }
console.log(tree.find(1)); // null

Both the PlayerTree and the Players directly address issue #36.

This PR may break a few things, but it has many benefits including modular development on the server, hooks for player serialization, and is test-ready. It isn't perfect, but it's a start.

What do you all think?

@jniles jniles changed the title Modularize the server; Add Player and PlayerTree classes Modularize the server; Add Player and BSTree classes Jun 5, 2015
@neslinesli93
Copy link
Contributor

This is the kind of commit we need to make the project shine!
Very cool work you did here, now we just need to get rid of socket.io and see if this game finally becomes playable over the net

@hunterpl
Copy link
Contributor

hunterpl commented Jun 5, 2015

@jniles Bug with joining players, if someone join and after first player join another player the first player see his nickname his colour of character and his how he move the cell. Waiting for the update :)

@jniles
Copy link
Contributor Author

jniles commented Jun 5, 2015

Bug is now fixed, as far as I can tell.

@jniles
Copy link
Contributor Author

jniles commented Jun 6, 2015

@igorantun , @huytd Would you mind taking a look at these ideas? There are lots of PRs, and it would be nice to clear them so we can start working on stability.

@giongto35
Copy link
Contributor

Is it a bit overkill? Number of players for this clone is never above 10. Even number player in agar io in each match is not over 200. Building a BST to improve from O(10) to O(log(10)) does not contribute much to this current situation and may cause the program slower when building a tree.
And in my opinion, about Data structure, a simple hash map might be better and faster than BST because the ID is unique for each player.

@jniles
Copy link
Contributor Author

jniles commented Jun 7, 2015

@giongto35 , the important aspect of this PR is the modularization and separation of concerns into a player class, and a container for players, moving that logic out of the server. Once an API is established to communicate between those entities, they can be implemented as trees, maps, etc.

I'm not sure overkill is a good reason to discard a data structure. A hash map is a perfectly acceptable alternative. The reasoning behind using a tree was to maintain a relative ordering between elements, hopefully taking advantage of mass or some other attribute, so we could quickly find the top 5 players in rapid time (see #119). This obviously isn't implemented in this PR, but I don't want to get too far ahead of the master branch and what other devs are doing.

@jniles jniles closed this Jun 8, 2015
@igorantun
Copy link
Collaborator

Why has this been closed?

@jniles jniles reopened this Jun 8, 2015
@jniles
Copy link
Contributor Author

jniles commented Jun 8, 2015

I've reopened it, apologies.

@igorantun
Copy link
Collaborator

Could you please fix the merge conflicts so I can merge this? Thanks in advance :)

@jniles
Copy link
Contributor Author

jniles commented Jun 8, 2015

I am closing this one, and will break these changes up into separate pull requests.

@jniles jniles closed this Jun 8, 2015
@igorantun
Copy link
Collaborator

Sure! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants