riatelab / magrit

Thematic cartography entirely in the browser ♥
https://magrit.cnrs.fr
GNU General Public License v3.0
128 stars 18 forks source link

Improve pictogram loading speed #110

Closed ArmelVidali closed 1 year ago

ArmelVidali commented 1 year ago

Hello,

I found an error when trying to add pictograms to a layer with a large amount of entities, the images don't seem to be loaded in time after clicking on "choose your symbols" then "valid" (on the warning message that states there are many features) to get the pictogram selection pop-up.

If you use a layer like world_countries_data with a large amount of entities and click fast enough (or use a slow enough internet connection) you will get a "Uncaught (in promise) TypeError: res_symbols[n_cat] is undefined", coming from the symbols_picto.js file, in the display_box_typo function.

If you wait a few seconds between clicking on "choose you symbols" and "valid", there is no error, seemingly because you let time for the client to upload all the images.

I had the same problem on an other branch modifying the image load and attribution on the pictogram panel, using compressed images (less than 5 octets - using country flags for european countries) seemed to solve this problem

mthh commented 1 year ago

Hello,

Thanks for raising this issue. I can easily reproduce it by selecting 'fast 3G' or 'slow 3g' in the network pane of the browser dev tools.

Indeed (as you may have seen) the data flow is as follow :

If you click on "Choose your symbols" before the symbols are loaded, it fails (without notifying the user), as you notified.

The original idea was to load pictograms only if the user need to make a map with pictograms (+ the fact that it plays nicely with the browser cache: once it has been loaded in your browser a first time, subsequent requests to the list_symbols.json and to all the png files use the cache, as in the screenshot below).

Capture du 2023-03-30 16-14-44

Magrit is already a bit heavy (about 1.5 Mb of data - JS, CSS, images, etc. - are transferred over the network when loading the page https://magrit.cnrs.fr/modules).

Since our set of symbols is currently about 215 Kb, maybe we should load all the symbols when loading the page ? (this will probably be unnoticeable for the user because we don't need to wait for the symbols to render the page)

ArmelVidali commented 1 year ago

Yes I think it would'nt make much difference at this stage, but I'm concerned it would collide with a feature I was waiting to submit (I was waiting for stack_labels to be finished but maybe I should do it already ?)

We worked on a feature allowing for an admin to put custom images on a server folder, that will be used to by the server to build a json just like list_symbols.json and load the images on the UI, and then associate rows and images on the pictogram selection popup based on a field and the name of the image (both have to match somehow)

For example with the european countries layer and flag images, the association would be made using each country code, to associate it's flag like this.

linked_images

If you would like this feature to be on magrit as well, uploading all the images might be too slow (for flags of all world country for example, even if each is octet-small, loading a lot of images could be slow ?) ?

Or maybe since the function loading the images return a promise, a solution could be found using an asynchronous design ? I am not experienced with async programming so I could'nt tell if it is appropriate.

mthh commented 1 year ago

Just to try it out, I modified when / where the pictograms are loaded. See the changes here : https://github.com/riatelab/magrit/compare/master...mthh:magrit:fix/improve-picto-loading?expand=1

By doing so, the pictograms are loaded when opening Magrit (but without blocking the loading and display of the page, i.e. the user can start using the app even if the pictograms do not have finished loading - it is still possible to trigger the bug with a connection slow enough, but it becomes more difficult).

Does this collide with the changes you are making?

The functionality you describe (matching categories with pictos based on category value and image name) is interesting and powerful but I'm afraid it's a bit of a niche feature (?).

I need to think about it a bit, but I'm not sure that we are inclined to merge such a functionality in Magrit, because it only works for very specific use cases, and because that could lead to wanting to add more and more pictograms to exploit this functionality (thus ending up loading a lot of useless data, especially since the pictogram functionality is probably not used very much in the various thematic cartography courses based on Magrit).

ArmelVidali commented 1 year ago

I think it's great ! And it doesn't collide with the new feature, getting extrabasemaps or uploading personal takes more time so a user probably won't encounter the error

Yes it's pretty niche and was made for a specific usecase

Adding the images on the server is probably the best for our use but if you end up wanting it merged it could be adapted by associating only the image uploaded by the user from the pictogram selection pop up.

mthh commented 1 year ago

I think we will load the pictograms when starting the application strating from the next version (for the reminder, this has been done in 132fec93 but it hasn't been merged yet).

mthh commented 1 year ago

These changes were included in dcf4904 and released in version 0.16.0.