earlSt1 / vtt-compendium-folders

Collapsable folders in the compendium directory and folder structures inside compendiums for FoundryVTT
21 stars 18 forks source link

Clicking on Actor in compendium takes a *very* long time when this mod is installed. #46

Closed MicahZoltu closed 3 years ago

MicahZoltu commented 3 years ago

I have no idea why Compendium Folders is causing this problem, but with it installed the first monster I click on when opening a compendium from a module takes 30-45 seconds to load. This seems to be related to either the specific monsters I have added to the compendium, though I'm not certain. I wish I was able to come up with a better repro case. Let me know if you want to hop on my server and experience the problem first hand.

  1. Install DDB-Import
  2. Configure to import into a shared compendium module.
  3. Check the following boxes: image image
  4. Import 1800 monsters (takes a long time)
  5. Install/enable Compendium Folders
  6. Open Monsters compendium.
  7. Click on a monsters.
  8. Notice that it takes about 30-45 seconds to load the monster.
  9. Click on another monster.
  10. Notice that it loads in a second or so.
  11. Close Monster compendium.
  12. Open Monster compendium.
  13. Click on a monster.
  14. Notice it takes about 30-45 seconds to load the monster again.
  15. Remove Compendium folders from the world.
  16. Repeat steps 6-8 but notice that the first monster loads quickly.
earlSt1 commented 3 years ago

Huh, so it does, even with 300 actors it's hanging a bit. I can see what's causing it too.

I could've sworn I implemented something to prevent the compendium folders code from running if there was no folders in the compendium.

Thanks for raising this. Shouldn't be too difficult to implement this check and once it's done I'll include this in the next release.

MicahZoltu commented 3 years ago

Root cause: https://github.com/earlSt1/vtt-compendium-folders/blob/1ef18345c16cd4c973352340b97677466eaec2c5/compendium-folders.js#L3777

Caching doesn't appear to work at all (at least for me), as I never get a cache hit when opening the compendium. Every time it goes into this code path, and that line takes a very long time to execute. Until that line finishes executing, I cannot actually do anything with the compendium. If I close the compendium window and re-open it, I have to go through the process again.

earlSt1 commented 3 years ago

The caching works by saving the folder data extracted when you first open the compendium. It only affects compendiums with foldders in them.

I've added a check to break out of the FIC code if no folders are detected in the compendium index. I've pushed this to the testing branch if you'd like to try it out.

You can find the testing manifest here: https://raw.githubusercontent.com/earlSt1/vtt-compendium-folders/master/test/module.json

I'd recommend doing a backup or at least saving your compendium folder export string before you try it out, just to be safe. The bulk of the changes in the next release affect how Folders For Compendiums are rendered.

MicahZoltu commented 3 years ago
Foundry VTT | Error thrown in hooked function renderSidebarTab foundry.js:2498:15
TypeError: folder is null
    addFolderLabel https://vtt.zoltu.io/modules/ddb-importer/dist/main.js:26238
    jQuery 2
    addFolderLabel https://vtt.zoltu.io/modules/ddb-importer/dist/main.js:26234
    renderSidebarTab https://vtt.zoltu.io/modules/ddb-importer/dist/main.js:26596
    _call https://vtt.zoltu.io/scripts/foundry.js:2496
    call https://vtt.zoltu.io/scripts/foundry.js:2481
MicahZoltu commented 3 years ago

So a breakpoint on that problematic line of code no longer gets hit, but loading the first monster still takes an abnormal amount of time while this plugin is loaded. 😢

MicahZoltu commented 3 years ago

New culprit: https://github.com/earlSt1/vtt-compendium-folders/blob/1ef18345c16cd4c973352340b97677466eaec2c5/compendium-folders.js#L3955

Edit: Fixed link to point at the right line.

earlSt1 commented 3 years ago

Ah, a compatibility issue with that ddb importer module, adding that to the todo list.

Also looks like you didnt get the latest version of the code, looking at this line. Maybe something cached when you reinstalled?

https://github.com/earlSt1/vtt-compendium-folders/blob/b87ac1c9947cdebebdbae36cd34bc7e77418d530/compendium-folders.js#L3772

MicahZoltu commented 3 years ago

I think I ended up grabbing a cache, future runs did not get the same error once I forced my browser to bust the cache. The pack.getContent() call is still a problem though, and happens when I have no other modules loaded except my shared compendium module.

MicahZoltu commented 3 years ago

A quick glance suggests you call Compendium.getContent() in a number of places in the code, and this operation will be very expensive for a large compendium. If possible, I recommend that any place where that is called other than as a first-run operation or a background call should be removed as it is going to lock up the UI if it happens in a blocking context.

MicahZoltu commented 3 years ago

Bleh, sorry. I grabbed the wrong line. This is the one that is causing long load times still: https://github.com/earlSt1/vtt-compendium-folders/blob/b87ac1c9947cdebebdbae36cd34bc7e77418d530/compendium-folders.js#L3965

earlSt1 commented 3 years ago

This is unfortunately a limitation to how I handle folders in compendiums. Since I cant make it a system setting like in the compendium directory (sharing a compendium wouldn't be possible then), I create temporary entities in the compendium which define folder data.

A thought I just had is to use the getIndex() method to retrieve the names and ids of entries, without loading everything. That way I can just loop through the folders and store their data.

And gah, i forgot that hook. Fortunately I can include the same check there (although its weird how its not happening for me)

MicahZoltu commented 3 years ago

A thought I just had is to use the getIndex() method to retrieve the names and ids of entries, without loading everything. That way I can just loop through the folders and store their data.

This would be great if it works, as I believe even for a very large compendium, getIndex() should be a pretty small payload compared to getContent().

earlSt1 commented 3 years ago

I've just pushed an update to the testing branch to break out of the getContent() call you mentioned (looking back on it i was using getContent() to check for folders, past me was very inefficient :smile:)

MicahZoltu commented 3 years ago

The original problem appears fixed. Thanks!

earlSt1 commented 3 years ago

Great, I'll leave this open until it makes its way into the next release.

The error from the ddb-importer module wont affect my module, but will probably break the importer from initializing correctly (its to do with the new way i render the compendium directory).

~Also wow, I just refactored the bits using getContent() and just using getIndex(). Its super fast (relies on looping through folders instead of looping through everything). Just pushed it to the testing branch if you want to get a sneak peak at it and maybe test it a bit.~ need some more work to get it working right

earlSt1 commented 3 years ago

Fixed in v2.2.0