Skip to content
This repository has been archived by the owner on Oct 2, 2021. It is now read-only.

Prepare for more scoring data. #286

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

clemens-tolboom
Copy link
Collaborator

@clemens-tolboom clemens-tolboom commented Jun 4, 2016

Description

This patch only prepare for storage of more score data. Others can gather the other data.

Our scores only contain the snake score but there is no context for the score

Competing snakes

In a dense arena (bigger snakes) you can score more easy and probably die too.

slither_io

Number of snakes

Playing with 400 or 500 does matter I guess

slither_io

TESTING STAGE

No test needed apart from does it break the score list.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read CONTRIBUTING.md
  • I fully understand the Github Flow and I'm merging into the develop branch , which is the default branch, and not the master branch
  • My code adheres to the code style of this project but most importantly of all to the things mentioned, which I have read, in CONTRIBUTING.md
  • If my change requires a change to the documentation, I have updated the documentation in /docs accordingly.
  • I have included the results of the test required.

@ermiyaeskandary
Copy link
Owner

Didn't see this - was commiting when this came up. @clemens-tolboom conflicts...

@clemens-tolboom clemens-tolboom force-pushed the feature/prepare-score-for-more branch from 8e81026 to 5bb660d Compare June 4, 2016 07:52

oContent.push('games played: ' + bot.scores.length);
oContent.push('a: ' + Math.round(
bot.scores.reduce(function (a, b) { return a + b; }) / (bot.scores.length)) +
bot.scores.reduce(function (a, b) { return a.score + b.score; }) / (bot.scores.length)) +

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 1222 exceeds the maximum line length of 100.
Unexpected space before function parentheses.

@clemens-tolboom
Copy link
Collaborator Author

Fixed.

But why does 753b883 has not an issue # attached?

@ermiyaeskandary
Copy link
Owner

@clemens-tolboom What issue ?

@clemens-tolboom
Copy link
Collaborator Author

Hmmm ... need to test my merge!

clemens-tolboom added a commit that referenced this pull request Jun 4, 2016
@clemens-tolboom
Copy link
Collaborator Author

@ermiyaeskandary each commit should be related to an issue / PR if possible. (me no saint either).

My guess is commit 753b883 is related to #260

Tested my rebased branch. No UI change but this is what I see

1 game
slither_io

2 game
slither_io

more games I see

slither_io

@ermiyaeskandary I did a bad use of reduce :(

var avg = Math.round(bot.scores.reduce(function (a, b) { return a.score + b.score; }) /
+                    (bot.scores.length));

does not seem a proper use of reduce by my change.

Argh. I must stop derailing my own work.

@ermiyaeskandary you mind to help?

@ermiyaeskandary
Copy link
Owner

@clemens-tolboom Did it work before the commit I did ?

@tjorim
Copy link
Collaborator

tjorim commented Jun 4, 2016

Take a look here: #203
Previously I was thinking of including the bot version, date... as well.

Also, I was wondering if we could gather the scores from users.
I think this would be possible using Google Spreadsheets, link a Google Script to it, and make simple POST to it with javascript.

clemens-tolboom added a commit that referenced this pull request Jun 4, 2016
@clemens-tolboom
Copy link
Collaborator Author

Fixed it. @tjorim thanks for the link.

I would propose to add a new 'pane' containing your data including browser and OS. Then bot bug reporters and developer can use that new 'pane'

This is now good to go (pfew)

@ermiyaeskandary
Copy link
Owner

ermiyaeskandary commented Jun 5, 2016

This does allow for more data - but do we need more data ?

@clemens-tolboom
Copy link
Collaborator Author

I said it @ several places. The score is not enough but I've updated the summary

@ermiyaeskandary
Copy link
Owner

@clemens-tolboom Correct but we only need average score to test the effectiveness. But I see where you're coming from - it needs to be a fair test.
Up for approvals - testing

@ChadSki
Copy link
Collaborator

ChadSki commented Jun 6, 2016

we only need average score to test the effectiveness

I'd argue we need the median, but yeah overall I agree - the other stats are distraction.

@clemens-tolboom
Copy link
Collaborator Author

I just played this

games played: 270
a: 177 m: 34
1. 9585
2. 5233
3. 5233
4. 5095
5. 5095
6. 1690
7. 1227
8. 381
9. 347
10. 347

and the stats gives me no clue on the why.

Having # players + highest 10 could give a clue about it's performance.

The stats could be csv exported into spreadsheet for all games.

About mentioned score ... I guess it's due to screensaver. So duration played would help a lot too.

@clemens-tolboom
Copy link
Collaborator Author

See #260 too

@ChadSki
Copy link
Collaborator

ChadSki commented Jun 7, 2016

I guess it's fair that more info would help us figure out why the bots scored as they did.

One thing I've noticed is that the average is always higher than the median. There seem to be lots of short runs with quick deaths.

@clemens-tolboom clemens-tolboom force-pushed the feature/prepare-score-for-more branch from 3e62892 to 4c4f66d Compare June 7, 2016 11:41
clemens-tolboom added a commit that referenced this pull request Jun 7, 2016
clemens-tolboom added a commit that referenced this pull request Jun 7, 2016
clemens-tolboom added a commit that referenced this pull request Jun 7, 2016
@@ -1456,5 +1464,6 @@ var userInterface = window.userInterface = (function() {
setInterval(userInterface.framesPerSecond.fpsTimer, 80);

// Start!
bot.startTime = Date.now();;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing whitespace after semicolon.
Unnecessary semicolon.

clemens-tolboom added a commit that referenced this pull request Jun 7, 2016
' m: ' + median);

for (var i = 0; i < bot.scores.length && i < 10; i++) {
oContent.push(i + 1 + '. ' + bot.scores[i]);
oContent.push(i + 1 + '. ' + bot.scores[i].score +
' in ' + Math.round(bot.scores[i].duration/1000) + 's');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infix operators must be spaced.

clemens-tolboom added a commit that referenced this pull request Jun 7, 2016
clemens-tolboom added a commit that referenced this pull request Jun 7, 2016
@clemens-tolboom
Copy link
Collaborator Author

I've added the game duration.

cursor_and_slither_io

and a bad table which could become nices (but that's another issue)

window.bot.scores.forEach(function(v,i,l){console.log(v.score, v.duration/1000, 'secs');});

2016-06-07 14:14:12.306 VM1901:1 1538 27.89 "secs"
2016-06-07 14:14:12.306 VM1901:1 683 39.555 "secs"
2016-06-07 14:14:12.307 VM1901:1 683 56.443 "secs"
2016-06-07 14:14:12.307 VM1901:1 192 31.203 "secs"
2016-06-07 14:14:12.307 VM1901:1 149 52.658 "secs"
2016-06-07 14:14:12.307 VM1901:1 149 22.178 "secs"
2016-06-07 14:14:12.307 VM1901:1 148 73.828 "secs"
2016-06-07 14:14:12.307 VM1901:1 41 28.04 "secs"
2016-06-07 14:14:12.307 VM1901:1 40 39.352 "secs"
2016-06-07 14:14:12.307 VM1901:1 32 45.235 "secs"
2016-06-07 14:14:12.307 VM1901:1 31 31.119 "secs"
2016-06-07 14:14:12.307 VM1901:1 31 27.08 "secs"
2016-06-07 14:14:12.308 VM1901:1 31 38.104 "secs"
2016-06-07 14:14:12.308 VM1901:1 31 4.985 "secs"
2016-06-07 14:14:12.308 VM1901:1 29 24.88 "secs"
2016-06-07 14:14:12.308 VM1901:1 29 25.861 "secs"
2016-06-07 14:14:12.308 VM1901:1 26 30.164 "secs"
2016-06-07 14:14:12.308 VM1901:1 24 20.951 "secs"
2016-06-07 14:14:12.308 VM1901:1 22 19.973 "secs"
2016-06-07 14:14:12.308 VM1901:1 22 39.998 "secs"
2016-06-07 14:14:12.308 VM1901:1 21 39.763 "secs"
2016-06-07 14:14:12.308 VM1901:1 21 42.832 "secs"
2016-06-07 14:14:12.308 VM1901:1 21 23.977 "secs"
2016-06-07 14:14:12.309 VM1901:1 21 23.783 "secs"
2016-06-07 14:14:12.309 VM1901:1 21 10.528 "secs"
2016-06-07 14:14:12.309 VM1901:1 18 39.906 "secs"
2016-06-07 14:14:12.309 VM1901:1 14 10.011 "secs"
2016-06-07 14:14:12.309 VM1901:1 13 18.985 "secs"
2016-06-07 14:14:12.309 VM1901:1 13 27.851 "secs"
2016-06-07 14:14:12.309 VM1901:1 12 13.749 "secs"
2016-06-07 14:14:12.309 VM1901:1 10 38.809 "secs"
2016-06-07 14:14:12.309 VM1901:1 10 17.019 "secs"

@tjorim
Copy link
Collaborator

tjorim commented Jun 8, 2016

👍

Approved with PullApprove

@ChadSki
Copy link
Collaborator

ChadSki commented Jun 8, 2016

Ping me this weekend if I haven't gotten around to testing+approving it by then. My PC's hard drive started clicking so I can't leave it running tests until I get a replacement drive.

@clemens-tolboom
Copy link
Collaborator Author

Please check whether by code flow for resetting is enough. There is

  • oefTimer
  • go
  • quickRespawn

My guess is we can do a refactoring through another issue with a new function resetGame of some sort.

@clemens-tolboom
Copy link
Collaborator Author

clemens-tolboom commented Jun 9, 2016

Can this be merged?

Future: found export to CSV https://jsfiddle.net/terryyounghk/kpegu/ which could export the terse version.

{
    score: score,
    duration: duration,
    topTenScore: [1,2,3,4,5,6,7,8,9,10],
    numberOfPlayers: 497
};

@ermiyaeskandary
Copy link
Owner

@clemens-tolboom Needs to be tested and then approved...

@ermiyaeskandary
Copy link
Owner

ermiyaeskandary commented Jun 9, 2016

Approved!

Approved with PullApprove

@ChadSki
Copy link
Collaborator

ChadSki commented Jun 9, 2016

All y'all doing the testing need to compare against the develop branch... standalone results don't mean much.

That said I expect this will not impact performance much or at all, but it's still worth testing.

Holding off my approval until my PC is repaired and can do testing, or if I see someone else has done a head-to-head comparison.

@clemens-tolboom clemens-tolboom force-pushed the feature/prepare-score-for-more branch from f9a92e4 to e3a98a6 Compare June 10, 2016 05:31
median = Math.round((bot.scores[Math.floor((bot.scores.length - 1) / 2)] +
bot.scores[Math.ceil((bot.scores.length - 1) / 2)]) / 2);

var avg = Math.round(bot.scores.reduce(function (a, b) { return a + b.score; }, 0) /

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unexpected space before function parentheses.

@ermiyaeskandary
Copy link
Owner

ermiyaeskandary commented Jun 10, 2016

That said I expect this will not impact performance much or at all, but it's still worth testing.

Testing today - I will only do about 10-15 games as it shouldn't affect performance at all like @ChadSki said but you never know - removing vars seems like it can't affect performance and it did so I'll test head to head with develop - @clemens-tolboom will probably be merged into master this week if there are no performance issues. 👍

@clemens-tolboom
Copy link
Collaborator Author

In https://gitter.im/ErmiyaEskandary/Slither.io-bot?at=575bd2ff97e1b2d245e1d05c @ermiyaeskandary reports a performance decrease but this PR but rechecking the code that could not be as the code AFAICT is ran outside the game.

@ermiyaeskandary
Copy link
Owner

ermiyaeskandary commented Jun 11, 2016

My results might have been anomalies but please correct me if I'm wrong:
bot.startTime = Date.now();
Doesn't this constantly check time and therefore might have an effect on the bot's performance which then has an effect on the efficiency?
@clemens-tolboom

@ChadSki
Copy link
Collaborator

ChadSki commented Jun 23, 2016

@clemens-tolboom the extra calculation in updateStats slows down the bot, because that function is called every loop as @ermiyaeskandary pointed out.

If you changed the code so that updateStats is called by a timer instead of calculating every frame, it might actually improve performance!

My PC is fixed so I can run tests overnight again.

});
userInterface.updateStats();
}

if (window.autoRespawn) {
bot.startTime = Date.now();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change suggests bad state management. We should have some bot states to check our code against

  • Not playing
  • Connecting
  • Connected?
  • Playing
  • Dying

@clemens-tolboom
Copy link
Collaborator Author

I think my reply on #286 (comment) is gone to /dev/null (wtf)

I have no clue of what is wrong apart for

  1. either I created a bug for my timing as you guys claim it's called every frame
  2. why introduce a timer? We should fix our state management aka have some bot states to check our code against
  3. Not playing
  4. Connecting
  5. Connected?
  6. Playing
  7. Dying

@tjorim
Copy link
Collaborator

tjorim commented Jun 24, 2016

I think we had all those states until the rewrite from @j-c-m, now the code
is a lot cleaner in my opinion though (but maybe not so handy).
On Jun 24, 2016 08:55, "Clemens Tolboom" [email protected] wrote:

I think my reply on #286 (comment)
#286 (comment)
is gone to /dev/null (wtf)

I have no clue of what is wrong apart for

either I created a bug for my timing as you guys claim it's called
every frame
2.

why introduce a timer? We should fix our state management aka have
some bot states to check our code against

  1. Not playing
  2. Connecting
  3. Connected?
  4. Playing
  5. Dying


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#286 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABPCHBPLZg4IbLXsynwZWM18MOOXF2P7ks5qO39HgaJpZM4IuE7c
.

@ChadSki
Copy link
Collaborator

ChadSki commented Jun 24, 2016

either I created a bug for my timing as you guys claim it's called every frame

The function was called every frame before your changes here. The difference is that you're adding more calculation to the function that's called every frame, which slows down the bot.

why introduce a timer?

Because we shouldn't be recalculating updateStats every frame in the first place.

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

Successfully merging this pull request may close these issues.

5 participants