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

Ch 2 - Add Performance measure visualisation #133

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

Conversation

Dragneel7
Copy link
Contributor

No description provided.

@Dragneel7
Copy link
Contributor Author

@redblobgames Please review this PR.
Thanks.

@redblobgames
Copy link
Contributor

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.

@Dragneel7
Copy link
Contributor Author

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.
The license as I read is for a free trial of 30 days, it is paid product , so maybe we should scrap canvas js and look for other free alternatives.

@Dragneel7
Copy link
Contributor Author

I just checked and the canvasJs lib is free for non-commercial purposes.
https://canvasjs.com/forums/topic/required-to-use-for-commercial-purpose/
So I guess we can stick to it.
@redblobgames

@Dragneel7
Copy link
Contributor Author

@redblobgames Thanks for the review, I have tried my best to solve the issues you pointed out.
I was able to plot the performance of both agents on the same graph but their performance is not real time any more but lags a bit.Please review whether this would work.

@redblobgames
Copy link
Contributor

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.

@Dragneel7
Copy link
Contributor Author

Dragneel7 commented Dec 27, 2017

@redblobgames how may I contact to AIMA team to ask them about the canvasJs license.
I don't know if it would be possible to run the whole thing using a single setInterval function as both the agents and the dirt function are supposed to work independently and at different intervals.But still I will look into it.

@redblobgames
Copy link
Contributor

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.

@Dragneel7
Copy link
Contributor Author

@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).

@redblobgames
Copy link
Contributor

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.

@Dragneel7
Copy link
Contributor Author

Dragneel7 commented Jan 5, 2018

@redblobgames Please review the PR. I am using Chart.js which is a free opensource library under MIT lisence.
The control over time as you mentioned, maybe we can add it as a feature in the future.
Thanks

@redblobgames
Copy link
Contributor

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

@Dragneel7
Copy link
Contributor Author

Dragneel7 commented Jan 11, 2018 via email

@Dragneel7
Copy link
Contributor Author

Dragneel7 commented Jan 11, 2018

@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.
I think the current diagram looks fine, yes if we had them side by side it would have been way better.
I don't think implementing the second part would be possible because the data is updated each time the plot function is called and it re-renders the whole graph. I have implemented it to the best of my capability, If there is any suggestion , I will try to implement it.(I will try to think of a solution to the above mentioned problem or can we add it as a feature update for future work)
Thanks.

@redblobgames
Copy link
Contributor

For side by side, something you can try: use the viewBox feature of SVG. If you have an <svg width="100" height="150"> and you want to shrink it, you can use <svg width="50 height="75" viewBox="0 0 100 150">.

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 viewBox and the size you want on the page for width and height.

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.

@Dragneel7
Copy link
Contributor Author

@redblobgames Thanks, that worked and I was able to align them side by side.
deepinscreenshot_select-area_20180118002507

I wasn't able to override the animation feature of ChartJs.
Are there any more improvements or is the visualization good to go.
Thanks.

@@ -233,7 +234,144 @@ function makeTableControlledDiagram() {
}
}

/*Control the daigram based on the performance parameters set
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check spelling, punctuation, spacing everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will surely correct them.

var agent2_interval;

//update agent1's environment
function update_agent1() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a local or a global?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no way to reuse the Chart object? The animations are very distracting!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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'; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what you're trying to do with the change to this function

Copy link
Contributor Author

@Dragneel7 Dragneel7 Jan 23, 2018

Choose a reason for hiding this comment

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

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.

@Dragneel7
Copy link
Contributor Author

@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.
I was able to update both the agents using the same function, I looked for the chartjs animation. I found a method which created a instance of chart and updated it through set interval which more or less was displaying the same way as before, so i decided to stick on the current method. Please tell me if the current work is up to standards.
Thanks.

@redblobgames redblobgames changed the title #130 Ch2: Add Performance measure visualisation Ch 2 - Add Performance measure visualisation Feb 6, 2019
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.

2 participants