Ch 2 - Add Performance measure visualisation #133
Conversation
|
@redblobgames Please review this PR. |
|
The visualization looks nice. To make the comparison easier you might try putting both lines into the same chart, with different color lines. The code is a mess (duplicate code, misspellings, spurious changes, inconsistent naming, etc.) but my guess is that you want comments on the visualization first, and then will clean up the code later. What is the license on canvasjs? We need to have the license information included in the third-party directory. |
|
I was aiming to show both the lines in the same chart but was unable as the information of scores were coming from different update functions at different time intervals so I had to duplicate the code, can you suggest something to solve this it would greatly improve the code quality. |
|
I just checked and the canvasJs lib is free for non-commercial purposes. |
|
@redblobgames Thanks for the review, I have tried my best to solve the issues you pointed out. |
|
The "free for non-commercial purposes" is a post on a forum. It is not a license. The software has a EULA which you need to convince AIMA to agree to: https://canvasjs.com/eula/chart/ One problem might be that AIMA (the textbook) is a commercial project. And since aimacode is an open source project under the MIT license, it allows commercial use, including use with the AIMA textbook. The CanvasJS license also states that we cannot distribute this code to software developers, and I think a lot of the purpose of aimacode is to distribute code to software developers, this may be a problem. I think you can talk to the AIMA team to see what they think. Or you can use d3.js (which this chapter already uses), since d3.js supports line charts. The combined chart looks much better! I think the reason you are seeing the lag in the chart is the way you have structured your code. You have lots of separate setInterval and setTimeout functions (8 of them I think), and making them synchronized is extra work. It would be much cleaner if you have one setInterval that controls the timeline. At each step, it can decide what to do about dirt, tell both vacuum agents to run, assign scores to the agents, draw the agents, and update the chart. That way everything is synchronized. If you have only one function that controls time you will be later able to add pause/play, and also fast forward or rewind controls. (For example, to fast forward 1000 steps you need to run 1000 steps of simulation but you would call the render function only 1 time.) I think control over time will probably be important for other charts on the page too. |
|
@redblobgames how may I contact to AIMA team to ask them about the canvasJs license. |
|
There is an email contact on http://aima.cs.berkeley.edu/gsoc-ideas.html or we can CC @norvig here and see if he is reading github comments. Peter: Dragneel7 would like to use a library which is marked as free for non-commercial use, and not to be distributed. If you are planning to use this in conjunction with your textbook, in any kind of commercial way, this may be a problem. And even if not, it seems like a license that does not allow distribution is going to be a problem. Thoughts? For the setInterval, I think you would set up an interval that corresponds to a discrete simulation of time. At some time ticks, the vacuum agent would run, and at other time ticks it would be waiting until it wakes up. One you have an abstraction over time then it enables many more visualizations, such as being able to rewind, or have multiple agents in parallel, or multiple random sequences in parallel, etc. For good reading on this subject, see Bret Victor's Ladder of Abstraction. |
|
@redblobgames Does the setInterval posses any issue in visualization, if so I will try to figure it out as I am planning to use d3 js for graph visualization(or should I wait for Norvig's reply). |
|
There shouldn't be any issues with setInterval and the visualization. d3 graph visualization is a good choice, as d3 is open source and the license allows us to use it. |
…into agent_performance
|
@redblobgames Please review the PR. I am using Chart.js which is a free opensource library under MIT lisence. |
|
Chart looks nice. Is there a way to disable the animation in chart.js? Every time you add a data point it seems to animate from zero, which makes it harder to see the data. It might be cool to put the agent animations side by side, by using half-sized svgs |
|
Sure I will put them side by side, chartjs will surely have the features to turn off the animation.
|
|
@redblobgames Changing the size of the svg won't help us as the size of the agent and the position of floors is the one we need to change, but as all the agents are rendered through a common make_diagram function changing the value will alter all the agents sizes which I don't believe is reasonable. |
|
For side by side, something you can try: use the This tells the browser that the SVG should be 50 wide, 75 high, but that the coordinate system inside the SVG should be 0-100 x 0-150. That way the original drawing that was designed for 100x150 will fit inside the SVG, but the browser will shrink it to be 50x75 instead. Use the original design size for the However, I don't remember if the code will allow you to do this easily. It is worth a look, as it would let you make them side by side more easily. |
|
@redblobgames Thanks, that worked and I was able to align them side by side. I wasn't able to override the animation feature of ChartJs. |
| @@ -233,7 +234,144 @@ function makeTableControlledDiagram() { | |||
| } | |||
| } | |||
|
|
|||
| /*Control the daigram based on the performance parameters set | |||
There was a problem hiding this comment.
Please check spelling, punctuation, spacing everywhere.
There was a problem hiding this comment.
Yes, I will surely correct them.
| var agent2_interval; | ||
|
|
||
| //update agent1's environment | ||
| function update_agent1() { |
There was a problem hiding this comment.
The code for updating agent1 and the code for updating agent2 are almost the same. These two should use shared code.
| } | ||
|
|
||
| function makefloordirty() { | ||
| floorNumber = Math.floor(Math.random() * 2); |
There was a problem hiding this comment.
Is this a local or a global?
There was a problem hiding this comment.
I made some mistakes while setting the scopes of the variable. I will correct it with other changes and be more careful in the future.
| var label = [0,1,2,3]; | ||
| var index = 3; | ||
| function plotPerformance(performanceAgent1, performanceAgent2){ | ||
| var performance_agent1 = performanceAgent1; |
There was a problem hiding this comment.
What is the purpose of this?
There was a problem hiding this comment.
I decided to put this array to make the y-axis of the chart more readable.
| var performance_agent2 = performanceAgent2; | ||
| var ctx = document.getElementById('chartContainer').getContext('2d'); | ||
| if(performance_agent1.length != 0 || performance_agent2.length != 0){ | ||
| var chart = new Chart(ctx, { |
There was a problem hiding this comment.
Is there no way to reuse the Chart object? The animations are very distracting!
There was a problem hiding this comment.
I tried to work on it as it was previously mentioned by you, but was unable to do so. I don't know if it is possible to do so.
| else if (world.location == 0) { return 'RIGHT'; } | ||
| else if (world.location == 1) { return 'LEFT'; } | ||
| else if (world.location == 1) { return'LEFT'; } |
There was a problem hiding this comment.
I don't understand what you're trying to do with the change to this function
There was a problem hiding this comment.
It was a mistake I might have committed when I was working to implement the visualization.
I will correct this and all the above mentioned.
Thanks for the Review.
|
@redblobgames Sorry for the late update to code, got a bit busy with college stuff. I have update most of the things you pointed out. |
redblobgames
changed the title
No description provided.