projectmesa / mesa-viz-tornado

Apache License 2.0
2 stars 8 forks source link

set() for local and package includes in ModularVisualization.py causes nondeterministic loading order. #24

Closed obbe79 closed 8 months ago

obbe79 commented 3 years ago

Describe the bug When loading the JavaScript files in the html template, their order is nondeterministic because local_includes and package_includes are converted in a set() in ModularVisualization. When there are dependencies between these javascript files, the resulting html may not preserve the correct order and the scripts will not work.

Expected behavior JavaScript files are loaded in the order given in the local_includes and package_includes variables inside the VisualizationElement subclass.

To Reproduce Build a VisualizationElement that needs multiple JavaScript files, you will see that the generated html files at run time randomly shuffles the scripts loading order.

Additional context I'm not sure if this is a bug. But I don't understand the reason for the set() inside ModularVisualization.py when collecting the local_includes and package_includes from the Visualization Elements.

Corvince commented 3 years ago

Good observation! Maybe it was originally implemented to avoid loading modules more than once? It's probably better to preserve order, though.

Do you have a specific use case where the order matters?

obbe79 commented 3 years ago

I'm using Leaflet.js and need to load some additional plug-ins. I realized about the order issue because sometimes, without any changes to the code, launching the server did not load the web interface properly because JavaScript could not find the declaration of some variables (specifically the L. in leaflet).

Anyway, at the moment I fixed it changing the set() to a list in the ModularVisualization. Probably a better fix would be using an OrderedDict(), to avoid duplicates and preserve order. Of course, the render method in tornado uses a list so the dict should then be converted to a list again.

In addition, I would also add the possibility to add css files (maybe modifying the template to accept a list of css_includes)

Corvince commented 3 years ago

Interesting, have you seen mesa_geo? It also provides leaflet maps, and I vaguely remember running into the same issue, but never figuring it out. So thanks for the late solution ;)

If you want to submit a PR: Normal dictionaries also preserve order since I think Python 3.6 so no need to use OrderedDict anymore.

obbe79 commented 3 years ago

Yes, I've considered mesa_geo however I am building an agent-based model for transportation problems and I need agents moving on the map. So, I drew some inspiration from mesa_geo but I believe that besides the map visualization my requirements are somewhat different. I will check it out once again to be sure I'm not missing something important.

As for the OrderedDict, I'm aware that now dicts preserve order, however I thought that for the sake of clarity it could be better to leave an explicit reference to the fact that the order is important