Open Dragneel7 opened 6 years ago
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. 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.
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
@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.
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. 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.
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.
@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
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. 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.
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.
@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. Are there any more improvements or is the visualization good to go. Thanks.
@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 Please review this PR. Thanks.